From d6ed405d0ceb4dac6fa1fd203ec0d5724aa066cc Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Wed, 20 Aug 2025 16:09:31 -0700 Subject: [PATCH 01/14] feat: multi edit and single search and replace tools --- core/config/load.ts | 4 +- core/config/profile/doLoadConfig.ts | 2 + core/config/yaml/loadYaml.ts | 4 +- core/index.d.ts | 2 + core/tools/builtIn.ts | 4 + core/tools/definitions/index.ts | 2 + core/tools/definitions/multiEdit.ts | 131 ++++++ .../tools/definitions/singleFindAndReplace.ts | 84 ++++ core/tools/index.ts | 62 +-- core/tools/searchWebGating.vitest.ts | 8 +- gui/src/redux/thunks/streamNormalInput.ts | 50 +-- gui/src/util/clientTools/callClientTool.ts | 12 + .../util/clientTools/multiEditImpl.test.ts | 390 ++++++++++++++++++ gui/src/util/clientTools/multiEditImpl.ts | 138 +++++++ .../singleFindAndReplaceImpl.test.ts | 332 +++++++++++++++ .../clientTools/singleFindAndReplaceImpl.ts | 82 ++++ 16 files changed, 1228 insertions(+), 79 deletions(-) create mode 100644 core/tools/definitions/multiEdit.ts create mode 100644 core/tools/definitions/singleFindAndReplace.ts create mode 100644 gui/src/util/clientTools/multiEditImpl.test.ts create mode 100644 gui/src/util/clientTools/multiEditImpl.ts create mode 100644 gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts create mode 100644 gui/src/util/clientTools/singleFindAndReplaceImpl.ts diff --git a/core/config/load.ts b/core/config/load.ts index dac5e217499..2be8fc332ac 100644 --- a/core/config/load.ts +++ b/core/config/load.ts @@ -62,7 +62,7 @@ import { } from "../util/paths"; import { localPathToUri } from "../util/pathToUri"; -import { getToolsForIde } from "../tools"; +import { getBaseToolDefinitions } from "../tools"; import { resolveRelativePathInDir } from "../util/ideUtils"; import { getWorkspaceRcConfigs } from "./json/loadRcConfigs"; import { modifyAnyConfigWithSharedConfig } from "./sharedConfig"; @@ -526,7 +526,7 @@ async function intermediateToFinalConfig({ const continueConfig: ContinueConfig = { ...config, contextProviders, - tools: await getToolsForIde(ide), + tools: getBaseToolDefinitions(), mcpServerStatuses: [], slashCommands: [], modelsByRole: { diff --git a/core/config/profile/doLoadConfig.ts b/core/config/profile/doLoadConfig.ts index 040fd3b41e3..3b705eebfca 100644 --- a/core/config/profile/doLoadConfig.ts +++ b/core/config/profile/doLoadConfig.ts @@ -287,6 +287,8 @@ export default async function doLoadConfig(options: { enableExperimentalTools: newConfig.experimental?.enableExperimentalTools ?? false, isSignedIn, + isRemote: await ide.isWorkspaceRemote(), + modelName: newConfig.selectedModelByRole.chat?.model, }), ); diff --git a/core/config/yaml/loadYaml.ts b/core/config/yaml/loadYaml.ts index 6772a2d0273..91921c3cb5f 100644 --- a/core/config/yaml/loadYaml.ts +++ b/core/config/yaml/loadYaml.ts @@ -35,7 +35,7 @@ import { modifyAnyConfigWithSharedConfig } from "../sharedConfig"; import { convertPromptBlockToSlashCommand } from "../../commands/slash/promptBlockSlashCommand"; import { slashCommandFromPromptFile } from "../../commands/slash/promptFileSlashCommand"; import { getControlPlaneEnvSync } from "../../control-plane/env"; -import { getToolsForIde } from "../../tools"; +import { getBaseToolDefinitions } from "../../tools"; import { getCleanUriPath } from "../../util/uri"; import { getAllDotContinueDefinitionFiles } from "../loadLocalAssistants"; import { unrollLocalYamlBlocks } from "./loadLocalYamlBlocks"; @@ -177,7 +177,7 @@ async function configYamlToContinueConfig(options: { const continueConfig: ContinueConfig = { slashCommands: [], - tools: await getToolsForIde(ide), + tools: getBaseToolDefinitions(), mcpServerStatuses: [], contextProviders: [], modelsByRole: { diff --git a/core/index.d.ts b/core/index.d.ts index 5a958a04e2e..5087e0c06cf 100644 --- a/core/index.d.ts +++ b/core/index.d.ts @@ -1112,6 +1112,8 @@ export interface ConfigDependentToolParams { rules: RuleWithSource[]; enableExperimentalTools: boolean; isSignedIn: boolean; + isRemote: boolean; + modelName: string | undefined; } export type GetTool = (params: ConfigDependentToolParams) => Tool; diff --git a/core/tools/builtIn.ts b/core/tools/builtIn.ts index c5c5da16c7c..6e8ed106234 100644 --- a/core/tools/builtIn.ts +++ b/core/tools/builtIn.ts @@ -2,6 +2,8 @@ export enum BuiltInToolNames { ReadFile = "read_file", EditExistingFile = "edit_existing_file", SearchAndReplaceInFile = "search_and_replace_in_file", + SingleFindAndReplace = "single_find_and_replace", + MultiEdit = "multi_edit", ReadCurrentlyOpenFile = "read_currently_open_file", CreateNewFile = "create_new_file", RunTerminalCommand = "run_terminal_command", @@ -25,4 +27,6 @@ export const BUILT_IN_GROUP_NAME = "Built-In"; export const CLIENT_TOOLS_IMPLS = [ BuiltInToolNames.EditExistingFile, BuiltInToolNames.SearchAndReplaceInFile, + BuiltInToolNames.SingleFindAndReplace, + BuiltInToolNames.MultiEdit, ]; diff --git a/core/tools/definitions/index.ts b/core/tools/definitions/index.ts index c1998444c92..6313eb9fc46 100644 --- a/core/tools/definitions/index.ts +++ b/core/tools/definitions/index.ts @@ -6,12 +6,14 @@ export { fetchUrlContentTool } from "./fetchUrlContent"; export { globSearchTool } from "./globSearch"; export { grepSearchTool } from "./grepSearch"; export { lsTool } from "./ls"; +export { multiEditTool } from "./multiEdit"; export { readCurrentlyOpenFileTool } from "./readCurrentlyOpenFile"; export { readFileTool } from "./readFile"; export { requestRuleTool } from "./requestRule"; export { runTerminalCommandTool } from "./runTerminalCommand"; export { searchAndReplaceInFileTool } from "./searchAndReplaceInFile"; export { searchWebTool } from "./searchWeb"; +export { singleFindAndReplaceTool } from "./singleFindAndReplace"; export { viewDiffTool } from "./viewDiff"; export { viewRepoMapTool } from "./viewRepoMap"; export { viewSubdirectoryTool } from "./viewSubdirectory"; diff --git a/core/tools/definitions/multiEdit.ts b/core/tools/definitions/multiEdit.ts new file mode 100644 index 00000000000..edacb19120c --- /dev/null +++ b/core/tools/definitions/multiEdit.ts @@ -0,0 +1,131 @@ +import { Tool } from "../.."; +import { BUILT_IN_GROUP_NAME, BuiltInToolNames } from "../builtIn"; + +export interface EditOperation { + old_string: string; + new_string: string; + replace_all?: boolean; +} + +export interface MultiEditArgs { + filepath: string; + edits: EditOperation[]; +} + +export const multiEditTool: Tool = { + type: "function", + displayTitle: "Multi Edit", + wouldLikeTo: "edit {{{ filepath }}}", + isCurrently: "editing {{{ filepath }}}", + hasAlready: "edited {{{ filepath }}}", + group: BUILT_IN_GROUP_NAME, + readonly: false, + isInstant: false, + function: { + name: BuiltInToolNames.MultiEdit, + description: `This is a tool for making multiple edits to a single file in one operation. It +is built on top of the single find and replace tool and allows you to perform multiple +find-and-replace operations efficiently. Prefer this tool over the single find and replace tool +when you need to make multiple edits to the same file. + +Before using this tool: + +1. Use the read_file tool to understand the file's contents and context +2. Verify the directory path is correct + +To make multiple file edits, provide the following: + +1. filepath: The path to the file to modify (relative to the root of the workspace) +2. edits: An array of edit operations to perform, where each edit contains: + - old_string: The text to replace (must match the file contents exactly, including all whitespace and indentation) + - new_string: The edited text to replace the old_string + - replace_all: Replace all occurrences of old_string. This parameter is optional and defaults to false. + +IMPORTANT: +- All edits are applied in sequence, in the order they are provided +- Each edit operates on the result of the previous edit +- All edits must be valid for the operation to succeed - if any edit fails, +none will be applied +- This tool is ideal when you need to make several changes to different parts +of the same file + +CRITICAL REQUIREMENTS: + +1. All edits follow the same requirements as the single find and replace tool +2. The edits are atomic - either all succeed or none are applied +3. Plan your edits carefully to avoid conflicts between sequential operations + +WARNING: + +- The tool will fail if edits.old_string doesn't match the file contents +exactly (including whitespace) +- The tool will fail if edits.old_string and edits.new_string are the same +- Since edits are applied in sequence, ensure that earlier edits don't affect +the text that later edits are trying to find + +When making edits: + +- Ensure all edits result in idiomatic, correct code +- Do not leave the code in a broken state +- Only use emojis if the user explicitly requests it. Avoid adding emojis to +files unless asked. +- Use replace_all for replacing and renaming strings across the file. This +parameter is useful if you want to rename a variable for instance. + +If you want to create a new file, use: +- A new file path +- First edit: empty old_string and the new file's contents as new_string +- Subsequent edits: normal edit operations on the created content`, + parameters: { + type: "object", + required: ["filepath", "edits"], + properties: { + filepath: { + type: "string", + description: + "The path to the file to modify, relative to the root of the workspace", + }, + edits: { + type: "array", + description: + "Array of edit operations to perform sequentially on the file", + items: { + type: "object", + required: ["old_string", "new_string"], + properties: { + old_string: { + type: "string", + description: "The text to replace", + }, + new_string: { + type: "string", + description: "The text to replace it with", + }, + replace_all: { + type: "boolean", + description: + "Replace all occurrences of old_string (default false)", + }, + }, + }, + }, + }, + }, + }, + systemMessageDescription: { + prefix: `To make multiple edits to a single file, use the ${BuiltInToolNames.MultiEdit} tool with a filepath (relative to the root of the workspace) and an array of edit operations. + + For example, you could respond with:`, + exampleArgs: [ + ["filepath", "path/to/file.ts"], + [ + "edits", + `[ + { "old_string": "const oldVar = 'value'", "new_string": "const newVar = 'updated'" }, + { "old_string": "oldFunction()", "new_string": "newFunction()", "replace_all": true } +]`, + ], + ], + }, + defaultToolPolicy: "allowedWithPermission", +}; diff --git a/core/tools/definitions/singleFindAndReplace.ts b/core/tools/definitions/singleFindAndReplace.ts new file mode 100644 index 00000000000..b5537d77523 --- /dev/null +++ b/core/tools/definitions/singleFindAndReplace.ts @@ -0,0 +1,84 @@ +import { Tool } from "../.."; +import { BUILT_IN_GROUP_NAME, BuiltInToolNames } from "../builtIn"; + +export interface SingleFindAndReplaceArgs { + filepath: string; + old_string: string; + new_string: string; + replace_all?: boolean; +} + +export const singleFindAndReplaceTool: Tool = { + type: "function", + displayTitle: "Find and Replace", + wouldLikeTo: "find and replace in {{{ filepath }}}", + isCurrently: "finding and replacing in {{{ filepath }}}", + hasAlready: "found and replaced in {{{ filepath }}}", + group: BUILT_IN_GROUP_NAME, + readonly: false, + isInstant: false, + function: { + name: BuiltInToolNames.SingleFindAndReplace, + description: `Performs exact string replacements in files. + +Usage: + +- You must use your \`read_file\` tool at least once in the conversation before +editing. This tool will error if you attempt an edit without reading the file. + +- When editing text from read_file tool output, ensure you preserve the exact +indentation (tabs/spaces) as it appears AFTER the line number prefix. The line +number prefix format is: spaces + line number + tab. Everything after that tab +is the actual file content to match. Never include any part of the line number +prefix in the old_string or new_string. + +- ALWAYS prefer editing existing files in the codebase. NEVER write new files +unless explicitly required. + +- Only use emojis if the user explicitly requests it. Avoid adding emojis to +files unless asked. + +- The edit will FAIL if \`old_string\` is not unique in the file. Either provide +a larger string with more surrounding context to make it unique or use +\`replace_all\` to change every instance of \`old_string\`. + +- Use \`replace_all\` for replacing and renaming strings across the file. This +parameter is useful if you want to rename a variable for instance.`, + parameters: { + type: "object", + required: ["filepath", "old_string", "new_string"], + properties: { + filepath: { + type: "string", + description: + "The path to the file to modify, relative to the root of the workspace", + }, + old_string: { + type: "string", + description: "The text to replace", + }, + new_string: { + type: "string", + description: + "The text to replace it with (must be different from old_string)", + }, + replace_all: { + type: "boolean", + description: "Replace all occurrences of old_string (default false)", + }, + }, + }, + }, + systemMessageDescription: { + prefix: `To perform exact string replacements in files, use the ${BuiltInToolNames.SingleFindAndReplace} tool with a filepath (relative to the root of the workspace) and the strings to find and replace. + + For example, you could respond with:`, + exampleArgs: [ + ["filepath", "path/to/file.ts"], + ["old_string", "const oldVariable = 'value'"], + ["new_string", "const newVariable = 'updated'"], + ["replace_all", "false"], + ], + }, + defaultToolPolicy: "allowedWithPermission", +}; diff --git a/core/tools/index.ts b/core/tools/index.ts index 850d89349cd..62033fa6660 100644 --- a/core/tools/index.ts +++ b/core/tools/index.ts @@ -1,12 +1,8 @@ -import { ConfigDependentToolParams, IDE, Tool } from ".."; +import { ConfigDependentToolParams, Tool } from ".."; import * as toolDefinitions from "./definitions"; // I'm writing these as functions because we've messed up 3 TIMES by pushing to const, causing duplicate tool definitions on subsequent config loads. - -// missing support for remote os calls: https://github.com/microsoft/vscode/issues/252269 -const getLocalOnlyToolDefinitions = () => [toolDefinitions.grepSearchTool]; - -const getBaseToolDefinitions = () => [ +export const getBaseToolDefinitions = () => [ toolDefinitions.readFileTool, toolDefinitions.createNewFileTool, toolDefinitions.runTerminalCommandTool, @@ -20,24 +16,38 @@ const getBaseToolDefinitions = () => [ export const getConfigDependentToolDefinitions = ( params: ConfigDependentToolParams, -): Tool[] => [ - toolDefinitions.requestRuleTool(params), - // Search and replace is now generally available - toolDefinitions.searchAndReplaceInFileTool, - // Keep edit file tool available for models that need it - toolDefinitions.editFileTool, - // Web search is only available for signed-in users - ...(params.isSignedIn ? [toolDefinitions.searchWebTool] : []), - ...(params.enableExperimentalTools - ? [ - toolDefinitions.viewRepoMapTool, - toolDefinitions.viewSubdirectoryTool, - toolDefinitions.codebaseTool, - ] - : []), -]; +): Tool[] => { + const { modelName, isSignedIn, enableExperimentalTools, isRemote } = params; + const tools: Tool[] = []; + + tools.push(toolDefinitions.requestRuleTool(params)); + + if (isSignedIn) { + // Web search is only available for signed-in users + tools.push(toolDefinitions.searchWebTool); + } + + if (enableExperimentalTools) { + tools.push( + toolDefinitions.viewRepoMapTool, + toolDefinitions.viewSubdirectoryTool, + toolDefinitions.codebaseTool, + ); + } + + // OLD SEARCH AND REPLACE IS CURRENTLY NOT USED + // toolDefinitions.searchAndReplaceInFileTool + if (modelName?.includes("claude") || modelName?.includes("gpt-5")) { + tools.push(toolDefinitions.multiEditTool); + } else { + tools.push(toolDefinitions.singleFindAndReplaceTool); + tools.push(toolDefinitions.editFileTool); + } + + // missing support for remote os calls: https://github.com/microsoft/vscode/issues/252269 + if (!isRemote) { + tools.push(toolDefinitions.grepSearchTool); + } -export const getToolsForIde = async (ide: IDE): Promise => - (await ide.isWorkspaceRemote()) - ? getBaseToolDefinitions() - : [...getBaseToolDefinitions(), ...getLocalOnlyToolDefinitions()]; + return tools; +}; diff --git a/core/tools/searchWebGating.vitest.ts b/core/tools/searchWebGating.vitest.ts index e68acdd52a3..4ebb6e5b45f 100644 --- a/core/tools/searchWebGating.vitest.ts +++ b/core/tools/searchWebGating.vitest.ts @@ -1,6 +1,6 @@ -import { test, expect } from "vitest"; -import { getConfigDependentToolDefinitions } from "./index"; +import { expect, test } from "vitest"; import { BuiltInToolNames } from "./builtIn"; +import { getConfigDependentToolDefinitions } from "./index"; test("searchWeb tool is only available when user is signed in", () => { // Test with signed-in user @@ -8,6 +8,8 @@ test("searchWeb tool is only available when user is signed in", () => { rules: [], enableExperimentalTools: false, isSignedIn: true, + isRemote: false, + modelName: "", }); const searchWebToolSignedIn = signedInTools.find( @@ -21,6 +23,8 @@ test("searchWeb tool is only available when user is signed in", () => { rules: [], enableExperimentalTools: false, isSignedIn: false, + isRemote: false, + modelName: "", }); const searchWebToolNotSignedIn = notSignedInTools.find( diff --git a/gui/src/redux/thunks/streamNormalInput.ts b/gui/src/redux/thunks/streamNormalInput.ts index 218883cb919..b17cf3d2992 100644 --- a/gui/src/redux/thunks/streamNormalInput.ts +++ b/gui/src/redux/thunks/streamNormalInput.ts @@ -1,8 +1,7 @@ import { createAsyncThunk, unwrapResult } from "@reduxjs/toolkit"; -import { LLMFullCompletionOptions, ModelDescription, Tool } from "core"; +import { LLMFullCompletionOptions, Tool } from "core"; import { getRuleId } from "core/llm/rules/getSystemMessageWithRules"; import { ToCoreProtocol } from "core/protocol"; -import { BuiltInToolNames } from "core/tools/builtIn"; import { selectActiveTools } from "../selectors/selectActiveTools"; import { selectUseSystemMessageTools } from "../selectors/selectUseSystemMessageTools"; import { selectSelectedChatModel } from "../slices/configSlice"; @@ -86,49 +85,6 @@ async function handleToolCallExecution( } } -/** - * Filters tools based on the selected model's capabilities. - * Returns either the edit file tool or search and replace tool, but not both. - */ -function filterToolsForModel( - tools: Tool[], - selectedModel: ModelDescription, -): Tool[] { - const editFileTool = tools.find( - (tool) => tool.function.name === BuiltInToolNames.EditExistingFile, - ); - const searchAndReplaceTool = tools.find( - (tool) => tool.function.name === BuiltInToolNames.SearchAndReplaceInFile, - ); - - // If we don't have both tools, return tools as-is - if (!editFileTool || !searchAndReplaceTool) { - return tools; - } - - // Determine which tool to use based on the model - const shouldUseFindReplace = shouldUseFindReplaceEdits(selectedModel); - - // Filter out the unwanted tool - return tools.filter((tool) => { - if (tool.function.name === BuiltInToolNames.EditExistingFile) { - return !shouldUseFindReplace; - } - if (tool.function.name === BuiltInToolNames.SearchAndReplaceInFile) { - return shouldUseFindReplace; - } - return true; - }); -} - -/** - * Determines whether to use search and replace tool instead of edit file - * Right now we only know that this is reliable with Claude models - */ -function shouldUseFindReplaceEdits({ model }: ModelDescription): boolean { - return model.includes("claude") || model.includes("gpt-5"); -} - export const streamNormalInput = createAsyncThunk< void, { @@ -146,8 +102,8 @@ export const streamNormalInput = createAsyncThunk< } // Get tools and filter them based on the selected model - const allActiveTools = selectActiveTools(state); - const activeTools = filterToolsForModel(allActiveTools, selectedChatModel); + const activeTools = selectActiveTools(state); + const supportsNativeTools = modelSupportsNativeTools(selectedChatModel); // Use the centralized selector to determine if system message tools should be used diff --git a/gui/src/util/clientTools/callClientTool.ts b/gui/src/util/clientTools/callClientTool.ts index b5ac685d305..755be6e7345 100644 --- a/gui/src/util/clientTools/callClientTool.ts +++ b/gui/src/util/clientTools/callClientTool.ts @@ -3,7 +3,9 @@ import { BuiltInToolNames } from "core/tools/builtIn"; import { IIdeMessenger } from "../../context/IdeMessenger"; import { AppThunkDispatch, RootState } from "../../redux/store"; import { editToolImpl } from "./editImpl"; +import { multiEditImpl } from "./multiEditImpl"; import { searchReplaceToolImpl } from "./searchReplaceImpl"; +import { singleFindAndReplaceImpl } from "./singleFindAndReplaceImpl"; export interface ClientToolExtras { getState: () => RootState; @@ -40,6 +42,16 @@ export async function callClientTool( case BuiltInToolNames.SearchAndReplaceInFile: output = await searchReplaceToolImpl(parsedArgs, toolCall.id, extras); break; + case BuiltInToolNames.SingleFindAndReplace: + output = await singleFindAndReplaceImpl( + parsedArgs, + toolCall.id, + extras, + ); + break; + case BuiltInToolNames.MultiEdit: + output = await multiEditImpl(parsedArgs, toolCall.id, extras); + break; default: throw new Error(`Invalid client tool name ${toolCall.function.name}`); } diff --git a/gui/src/util/clientTools/multiEditImpl.test.ts b/gui/src/util/clientTools/multiEditImpl.test.ts new file mode 100644 index 00000000000..edbba81fe99 --- /dev/null +++ b/gui/src/util/clientTools/multiEditImpl.test.ts @@ -0,0 +1,390 @@ +import * as ideUtils from "core/util/ideUtils"; +import { beforeEach, describe, expect, it, Mock, vi } from "vitest"; +import { ClientToolExtras } from "./callClientTool"; +import { multiEditImpl } from "./multiEditImpl"; + +vi.mock("uuid", () => ({ + v4: vi.fn(() => "test-uuid"), +})); + +vi.mock("core/util/ideUtils", () => ({ + resolveRelativePathInDir: vi.fn(), +})); + +describe("multiEditImpl", () => { + let mockExtras: ClientToolExtras; + let mockResolveRelativePathInDir: Mock; + + beforeEach(() => { + vi.clearAllMocks(); + + mockResolveRelativePathInDir = vi.mocked(ideUtils.resolveRelativePathInDir); + + mockExtras = { + getState: vi.fn(() => ({ + config: { config: { allowAnonymousTelemetry: false } }, + })) as any, + dispatch: vi.fn() as any, + ideMessenger: { + ide: { readFile: vi.fn() }, + request: vi.fn(), + } as any, + }; + }); + + describe("argument validation", () => { + it("should throw if filepath is missing", async () => { + await expect( + multiEditImpl({ edits: [] }, "id", mockExtras), + ).rejects.toThrow("filepath is required"); + }); + + it("should throw if edits array is empty", async () => { + await expect( + multiEditImpl({ filepath: "test.txt", edits: [] }, "id", mockExtras), + ).rejects.toThrow( + "edits array is required and must contain at least one edit", + ); + }); + + it("should throw if edit has invalid old_string", async () => { + await expect( + multiEditImpl( + { + filepath: "test.txt", + edits: [{ old_string: null, new_string: "test" }], + }, + "id", + mockExtras, + ), + ).rejects.toThrow("Edit 1: old_string is required"); + }); + + it("should throw if edit has undefined new_string", async () => { + await expect( + multiEditImpl( + { + filepath: "test.txt", + edits: [{ old_string: "test", new_string: undefined }], + }, + "id", + mockExtras, + ), + ).rejects.toThrow("Edit 1: new_string is required"); + }); + + it("should throw if old_string equals new_string", async () => { + await expect( + multiEditImpl( + { + filepath: "test.txt", + edits: [{ old_string: "same", new_string: "same" }], + }, + "id", + mockExtras, + ), + ).rejects.toThrow("Edit 1: old_string and new_string must be different"); + }); + }); + + describe("sequential edits", () => { + beforeEach(() => { + mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + }); + + it("should apply single edit", async () => { + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("Hello world"); + + await multiEditImpl( + { + filepath: "file.txt", + edits: [{ old_string: "Hello", new_string: "Hi" }], + }, + "id", + mockExtras, + ); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "id", + text: "Hi world", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }, + ); + }); + + it("should apply multiple edits sequentially", async () => { + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("Hello world\nGoodbye world"); + + await multiEditImpl( + { + filepath: "file.txt", + edits: [ + { old_string: "Hello", new_string: "Hi" }, + { old_string: "world", new_string: "universe", replace_all: true }, + ], + }, + "id", + mockExtras, + ); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "id", + text: "Hi universe\nGoodbye universe", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }, + ); + }); + + it("should handle edits that depend on previous edits", async () => { + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("const x = 1;"); + + await multiEditImpl( + { + filepath: "file.txt", + edits: [ + { old_string: "const x", new_string: "let x" }, + { old_string: "let x = 1;", new_string: "let x = 2;" }, + ], + }, + "id", + mockExtras, + ); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "id", + text: "let x = 2;", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }, + ); + }); + + it("should throw if string not found in edit sequence", async () => { + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("Hello world"); + + await expect( + multiEditImpl( + { + filepath: "file.txt", + edits: [ + { old_string: "Hello", new_string: "Hi" }, + { old_string: "not found", new_string: "test" }, + ], + }, + "id", + mockExtras, + ), + ).rejects.toThrow('Edit 2: String not found in file: "not found"'); + }); + + it("should throw if multiple occurrences without replace_all", async () => { + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("test test test"); + + await expect( + multiEditImpl( + { + filepath: "file.txt", + edits: [{ old_string: "test", new_string: "replaced" }], + }, + "id", + mockExtras, + ), + ).rejects.toThrow('Edit 1: String "test" appears 3 times'); + }); + }); + + describe("file creation", () => { + it("should create new file with empty old_string", async () => { + mockResolveRelativePathInDir.mockResolvedValue(null); + + await multiEditImpl( + { + filepath: "new.txt", + edits: [{ old_string: "", new_string: "New content\nLine 2" }], + }, + "id", + mockExtras, + ); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "id", + text: "New content\nLine 2", + filepath: "new.txt", + isSearchAndReplace: true, + }, + ); + }); + + it("should create and edit new file", async () => { + mockResolveRelativePathInDir.mockResolvedValue(null); + + await multiEditImpl( + { + filepath: "new.txt", + edits: [ + { old_string: "", new_string: "Hello world" }, + { old_string: "world", new_string: "universe" }, + ], + }, + "id", + mockExtras, + ); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "id", + text: "Hello universe", + filepath: "new.txt", + isSearchAndReplace: true, + }, + ); + }); + }); + + describe("replace_all functionality", () => { + beforeEach(() => { + mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + }); + + it("should replace all occurrences when specified", async () => { + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("foo bar foo baz foo"); + + await multiEditImpl( + { + filepath: "file.txt", + edits: [{ old_string: "foo", new_string: "qux", replace_all: true }], + }, + "id", + mockExtras, + ); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "id", + text: "qux bar qux baz qux", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }, + ); + }); + + it("should handle mixed replace_all settings", async () => { + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("x y x z x"); + + await multiEditImpl( + { + filepath: "file.txt", + edits: [ + { old_string: "x", new_string: "a", replace_all: true }, + { old_string: "y", new_string: "b" }, + ], + }, + "id", + mockExtras, + ); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "id", + text: "a b a z a", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }, + ); + }); + }); + + describe("error handling", () => { + it("should wrap readFile errors", async () => { + mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockRejectedValue(new Error("Read failed")); + + await expect( + multiEditImpl( + { + filepath: "file.txt", + edits: [{ old_string: "test", new_string: "new" }], + }, + "id", + mockExtras, + ), + ).rejects.toThrow("Failed to apply multi edit: Read failed"); + }); + + it("should wrap applyToFile errors", async () => { + mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test"); + mockExtras.ideMessenger.request = vi + .fn() + .mockRejectedValue(new Error("Write failed")); + + await expect( + multiEditImpl( + { + filepath: "file.txt", + edits: [{ old_string: "test", new_string: "new" }], + }, + "id", + mockExtras, + ), + ).rejects.toThrow("Failed to apply multi edit: Write failed"); + }); + }); + + describe("return value", () => { + it("should return correct structure", async () => { + mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test"); + + const result = await multiEditImpl( + { + filepath: "file.txt", + edits: [{ old_string: "test", new_string: "new" }], + }, + "id", + mockExtras, + ); + + expect(result).toEqual({ + respondImmediately: false, + output: undefined, + }); + }); + }); +}); diff --git a/gui/src/util/clientTools/multiEditImpl.ts b/gui/src/util/clientTools/multiEditImpl.ts new file mode 100644 index 00000000000..77b4f867679 --- /dev/null +++ b/gui/src/util/clientTools/multiEditImpl.ts @@ -0,0 +1,138 @@ +import { resolveRelativePathInDir } from "core/util/ideUtils"; +import { v4 as uuid } from "uuid"; +import { ClientToolImpl } from "./callClientTool"; + +interface EditOperation { + old_string: string; + new_string: string; + replace_all?: boolean; +} + +function validateEdits(edits: EditOperation[]): void { + for (let i = 0; i < edits.length; i++) { + const edit = edits[i]; + if (!edit.old_string && edit.old_string !== "") { + throw new Error(`Edit ${i + 1}: old_string is required`); + } + if (edit.new_string === undefined) { + throw new Error(`Edit ${i + 1}: new_string is required`); + } + if (edit.old_string === edit.new_string) { + throw new Error( + `Edit ${i + 1}: old_string and new_string must be different`, + ); + } + } +} + +function applyEdit( + content: string, + edit: EditOperation, + editIndex: number, + isFirstEditOfNewFile: boolean, +): string { + const { old_string, new_string, replace_all = false } = edit; + + // For new file creation, the first edit can have empty old_string + if (isFirstEditOfNewFile && old_string === "") { + return new_string; + } + + // Check if old_string exists in current content + if (!content.includes(old_string)) { + throw new Error( + `Edit ${editIndex + 1}: String not found in file: "${old_string}"`, + ); + } + + if (replace_all) { + // Replace all occurrences + return content.split(old_string).join(new_string); + } else { + // Replace only the first occurrence, but check for uniqueness + const occurrences = content.split(old_string).length - 1; + if (occurrences > 1) { + throw new Error( + `Edit ${editIndex + 1}: String "${old_string}" appears ${occurrences} times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.`, + ); + } + return content.replace(old_string, new_string); + } +} + +export const multiEditImpl: ClientToolImpl = async ( + args, + toolCallId, + extras, +) => { + const { filepath, edits } = args; + + const streamId = uuid(); + + // Validate arguments + if (!filepath) { + throw new Error("filepath is required"); + } + if (!edits || !Array.isArray(edits) || edits.length === 0) { + throw new Error( + "edits array is required and must contain at least one edit", + ); + } + + // Validate each edit operation + validateEdits(edits); + + // Check if this is creating a new file (first edit has empty old_string) + const isCreatingNewFile = edits[0].old_string === ""; + + // Resolve the file path + const resolvedFilepath = await resolveRelativePathInDir( + filepath, + extras.ideMessenger.ide, + ); + + // For new files, resolvedFilepath might be null, so we construct the path + const targetFilepath = resolvedFilepath || filepath; + + if (!isCreatingNewFile) { + // For existing files, check if file exists + if (!resolvedFilepath) { + throw new Error(`File ${filepath} does not exist`); + } + } + + try { + // Read current file content (or start with empty for new files) + let currentContent = ""; + if (!isCreatingNewFile) { + currentContent = await extras.ideMessenger.ide.readFile(targetFilepath); + } + + let newContent = currentContent; + + // Apply all edits sequentially + for (let i = 0; i < edits.length; i++) { + const isFirstEditOfNewFile = i === 0 && isCreatingNewFile; + newContent = applyEdit(newContent, edits[i], i, isFirstEditOfNewFile); + } + + // Apply the changes to the file + await extras.ideMessenger.request("applyToFile", { + streamId, + toolCallId, + text: newContent, + filepath: targetFilepath, + isSearchAndReplace: true, + }); + + // Return success - applyToFile will handle the completion state + return { + respondImmediately: false, // Let apply state handle completion + output: undefined, + }; + } catch (error) { + throw new Error( + `Failed to apply multi edit: ${error instanceof Error ? error.message : String(error)}`, + ); + } +}; diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts new file mode 100644 index 00000000000..fb3c45bbcc1 --- /dev/null +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts @@ -0,0 +1,332 @@ +import { describe, it, expect, beforeEach, vi, Mock } from "vitest"; +import { v4 as uuid } from "uuid"; +import { singleFindAndReplaceImpl } from "./singleFindAndReplaceImpl"; +import { ClientToolExtras } from "./callClientTool"; +import * as ideUtils from "core/util/ideUtils"; + +vi.mock("uuid", () => ({ + v4: vi.fn(() => "test-uuid"), +})); + +vi.mock("core/util/ideUtils", () => ({ + resolveRelativePathInDir: vi.fn(), +})); + +describe("singleFindAndReplaceImpl", () => { + let mockExtras: ClientToolExtras; + let mockResolveRelativePathInDir: Mock; + + beforeEach(() => { + vi.clearAllMocks(); + + mockResolveRelativePathInDir = vi.mocked(ideUtils.resolveRelativePathInDir); + + mockExtras = { + getState: vi.fn(() => ({ + config: { + config: { + allowAnonymousTelemetry: false, + }, + }, + })) as any, + dispatch: vi.fn() as any, + ideMessenger: { + ide: { + readFile: vi.fn(), + }, + request: vi.fn(), + } as any, + }; + }); + + describe("argument validation", () => { + it("should throw error if filepath is missing", async () => { + const args = { + old_string: "test", + new_string: "replacement", + }; + + await expect( + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + ).rejects.toThrow("filepath is required"); + }); + + it("should throw error if old_string is missing", async () => { + const args = { + filepath: "test.txt", + new_string: "replacement", + }; + + await expect( + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + ).rejects.toThrow("old_string is required"); + }); + + it("should throw error if new_string is missing", async () => { + const args = { + filepath: "test.txt", + old_string: "test", + }; + + await expect( + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + ).rejects.toThrow("new_string is required"); + }); + + it("should throw error if old_string and new_string are the same", async () => { + const args = { + filepath: "test.txt", + old_string: "same", + new_string: "same", + }; + + await expect( + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + ).rejects.toThrow("old_string and new_string must be different"); + }); + }); + + describe("file resolution", () => { + it("should throw error if file does not exist", async () => { + mockResolveRelativePathInDir.mockResolvedValue(null); + + const args = { + filepath: "nonexistent.txt", + old_string: "test", + new_string: "replacement", + }; + + await expect( + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + ).rejects.toThrow("File nonexistent.txt does not exist"); + }); + + it("should resolve relative file paths", async () => { + mockResolveRelativePathInDir.mockResolvedValue("/absolute/path/test.txt"); + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test content"); + + const args = { + filepath: "test.txt", + old_string: "test", + new_string: "replacement", + }; + + await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); + + expect(mockResolveRelativePathInDir).toHaveBeenCalledWith( + "test.txt", + mockExtras.ideMessenger.ide + ); + expect(mockExtras.ideMessenger.ide.readFile).toHaveBeenCalledWith( + "/absolute/path/test.txt" + ); + }); + }); + + describe("string replacement", () => { + beforeEach(() => { + mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + }); + + it("should throw error if old_string is not found in file", async () => { + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("different content"); + + const args = { + filepath: "file.txt", + old_string: "not found", + new_string: "replacement", + }; + + await expect( + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + ).rejects.toThrow("String not found in file: not found"); + }); + + it("should replace single occurrence", async () => { + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( + "Hello world\nThis is a test file\nGoodbye world" + ); + + const args = { + filepath: "file.txt", + old_string: "Hello world", + new_string: "Hi there", + }; + + await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith("applyToFile", { + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "Hi there\nThis is a test file\nGoodbye world", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); + }); + + it("should throw error if old_string appears multiple times and replace_all is false", async () => { + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( + "Hello world\nThis is a test file\nGoodbye world" + ); + + const args = { + filepath: "file.txt", + old_string: "world", + new_string: "universe", + replace_all: false, + }; + + await expect( + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + ).rejects.toThrow( + 'String "world" appears 2 times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.' + ); + }); + + it("should replace all occurrences when replace_all is true", async () => { + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( + "Hello world\nThis is a test file\nGoodbye world" + ); + + const args = { + filepath: "file.txt", + old_string: "world", + new_string: "universe", + replace_all: true, + }; + + await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith("applyToFile", { + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "Hello universe\nThis is a test file\nGoodbye universe", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); + }); + + it("should handle empty new_string (deletion)", async () => { + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( + "Hello world\nThis is a test file" + ); + + const args = { + filepath: "file.txt", + old_string: "Hello ", + new_string: "", + }; + + await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith("applyToFile", { + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "world\nThis is a test file", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); + }); + + it("should handle special characters in strings", async () => { + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( + 'const regex = /[a-z]+/g;\nconst text = "Hello $world"' + ); + + const args = { + filepath: "file.txt", + old_string: '"Hello $world"', + new_string: '"Hi $universe"', + }; + + await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith("applyToFile", { + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: 'const regex = /[a-z]+/g;\nconst text = "Hi $universe"', + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); + }); + + it("should preserve whitespace and indentation", async () => { + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( + "function test() {\n const value = 'old';\n return value;\n}" + ); + + const args = { + filepath: "file.txt", + old_string: " const value = 'old';", + new_string: " const value = 'new';", + }; + + await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); + + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith("applyToFile", { + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "function test() {\n const value = 'new';\n return value;\n}", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); + }); + }); + + describe("return value", () => { + it("should return correct response structure", async () => { + mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test content"); + + const args = { + filepath: "file.txt", + old_string: "test", + new_string: "replacement", + }; + + const result = await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); + + expect(result).toEqual({ + respondImmediately: false, + output: undefined, + }); + }); + }); + + describe("error handling", () => { + it("should wrap and rethrow errors from readFile", async () => { + mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + mockExtras.ideMessenger.ide.readFile = vi.fn().mockRejectedValue( + new Error("Permission denied") + ); + + const args = { + filepath: "file.txt", + old_string: "test", + new_string: "replacement", + }; + + await expect( + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + ).rejects.toThrow("Failed to apply find and replace: Permission denied"); + }); + + it("should wrap and rethrow errors from applyToFile", async () => { + mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test content"); + mockExtras.ideMessenger.request = vi.fn().mockRejectedValue( + new Error("Write failed") + ); + + const args = { + filepath: "file.txt", + old_string: "test", + new_string: "replacement", + }; + + await expect( + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + ).rejects.toThrow("Failed to apply find and replace: Write failed"); + }); + }); +}); \ No newline at end of file diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts new file mode 100644 index 00000000000..3793fec5695 --- /dev/null +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts @@ -0,0 +1,82 @@ +import { resolveRelativePathInDir } from "core/util/ideUtils"; +import { v4 as uuid } from "uuid"; +import { ClientToolImpl } from "./callClientTool"; + +export const singleFindAndReplaceImpl: ClientToolImpl = async ( + args, + toolCallId, + extras, +) => { + const { filepath, old_string, new_string, replace_all = false } = args; + + const streamId = uuid(); + + // Validate arguments + if (!filepath) { + throw new Error("filepath is required"); + } + if (!old_string) { + throw new Error("old_string is required"); + } + if (new_string === undefined) { + throw new Error("new_string is required"); + } + if (old_string === new_string) { + throw new Error("old_string and new_string must be different"); + } + + // Resolve the file path + const resolvedFilepath = await resolveRelativePathInDir( + filepath, + extras.ideMessenger.ide, + ); + if (!resolvedFilepath) { + throw new Error(`File ${filepath} does not exist`); + } + + try { + // Read the current file content + const originalContent = + await extras.ideMessenger.ide.readFile(resolvedFilepath); + + // Check if old_string exists in the file + if (!originalContent.includes(old_string)) { + throw new Error(`String not found in file: ${old_string}`); + } + + let newContent: string; + + if (replace_all) { + // Replace all occurrences + newContent = originalContent.split(old_string).join(new_string); + } else { + // Replace only the first occurrence + const occurrences = originalContent.split(old_string).length - 1; + if (occurrences > 1) { + throw new Error( + `String "${old_string}" appears ${occurrences} times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.`, + ); + } + newContent = originalContent.replace(old_string, new_string); + } + + // Apply the changes to the file + await extras.ideMessenger.request("applyToFile", { + streamId, + toolCallId, + text: newContent, + filepath: resolvedFilepath, + isSearchAndReplace: true, + }); + + // Return success - applyToFile will handle the completion state + return { + respondImmediately: false, // Let apply state handle completion + output: undefined, + }; + } catch (error) { + throw new Error( + `Failed to apply find and replace: ${error instanceof Error ? error.message : String(error)}`, + ); + } +}; From efa6328643e05ff634a38718f9a83b723a6c2fed Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Wed, 20 Aug 2025 17:18:23 -0700 Subject: [PATCH 02/14] feat: UIs and add single replace to default tools --- core/llm/toolSupport.ts | 1 + core/tools/index.ts | 2 +- gui/src/pages/gui/ToolCallDiv/EditFile.tsx | 2 +- .../FunctionSpecificToolCallDiv.tsx | 39 +++++++++++++++++++ gui/src/pages/gui/ToolCallDiv/index.tsx | 6 ++- gui/src/util/toolCallState.ts | 12 +++--- 6 files changed, 52 insertions(+), 10 deletions(-) diff --git a/core/llm/toolSupport.ts b/core/llm/toolSupport.ts index 7c89dbfa4a8..c0aa84b85f0 100644 --- a/core/llm/toolSupport.ts +++ b/core/llm/toolSupport.ts @@ -343,6 +343,7 @@ export function isRecommendedAgentModel(modelName: string): boolean { [/deepseek/, /r1|reasoner/], [/gemini/, /2\.5/, /pro/], [/gpt/, /4/], + [/gpt-5/], [/claude/, /sonnet/, /3\.5|3\.7|3-5|3-7|-4/], [/claude/, /opus/, /-4/], ]; diff --git a/core/tools/index.ts b/core/tools/index.ts index 62033fa6660..fa66c4c915e 100644 --- a/core/tools/index.ts +++ b/core/tools/index.ts @@ -12,6 +12,7 @@ export const getBaseToolDefinitions = () => [ toolDefinitions.lsTool, toolDefinitions.createRuleBlock, toolDefinitions.fetchUrlContentTool, + toolDefinitions.singleFindAndReplaceTool, ]; export const getConfigDependentToolDefinitions = ( @@ -40,7 +41,6 @@ export const getConfigDependentToolDefinitions = ( if (modelName?.includes("claude") || modelName?.includes("gpt-5")) { tools.push(toolDefinitions.multiEditTool); } else { - tools.push(toolDefinitions.singleFindAndReplaceTool); tools.push(toolDefinitions.editFileTool); } diff --git a/gui/src/pages/gui/ToolCallDiv/EditFile.tsx b/gui/src/pages/gui/ToolCallDiv/EditFile.tsx index dbab7a18df6..3cbcab061c6 100644 --- a/gui/src/pages/gui/ToolCallDiv/EditFile.tsx +++ b/gui/src/pages/gui/ToolCallDiv/EditFile.tsx @@ -17,7 +17,7 @@ export function EditFile(props: EditToolCallProps) { return null; } - const src = `\`\`\`${getMarkdownLanguageTagForFile(props.relativeFilePath)} ${props.relativeFilePath}\n${props.changes}\n\`\`\``; + const src = `\`\`\`${getMarkdownLanguageTagForFile(props.relativeFilePath)} ${props.relativeFilePath}${props.changes ? "\n" + props.changes + "\n" : ""}\`\`\``; return ( ); + case BuiltInToolNames.SingleFindAndReplace: + const fakeSearchReplaceBlock = `===== SEARCH +${args?.old_string ?? ""} +===== +${args?.new_string ?? ""} +===== REPLACE`; + + return ( + + ); + case BuiltInToolNames.MultiEdit: + const fakeSearchReplaceBlocks = + (args?.edits as { old_string: string; new_string: string }[]) + ?.map( + (edit) => `===== SEARCH +${edit?.old_string ?? ""} +===== +${edit?.new_string ?? ""} +===== REPLACE`, + ) + .join("\n\n---\n\n") ?? ""; + + return ( + + ); case BuiltInToolNames.RunTerminalCommand: return ( "{"file": "file1", "line": 1}" - mergedArgs = argsDelta; - } else if (!currentArgs.startsWith(argsDelta)) { - // Case where model streams in args in parts e.g. "{"file": "file1"" -> ", "line": 1}" - mergedArgs = currentArgs + argsDelta; - } + // Model streams in args in parts e.g. "{"file": "file1"" -> ", "line": 1}" + mergedArgs = currentArgs + argsDelta; + + // Note, removed case where model progresssively streams args but full args each time e.g. "{"file": "file1"}" -> "{"file": "file1", "line": 1}" + // Because no apis do this and difficult to detect reliably } const [_, parsedArgs] = incrementalParseJson(mergedArgs || "{}"); From 54c16a3f03b2aa1f1f12118196b3d092943022f6 Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Wed, 20 Aug 2025 17:45:16 -0700 Subject: [PATCH 03/14] fix: tests and prettier --- .../tools/definitions/toolDefinitions.test.ts | 2 + .../vscode/src/diff/vertical/handler.ts | 4 +- .../components/modelSelection/ModelCard.tsx | 4 +- .../singleFindAndReplaceImpl.test.ts | 189 ++++++++++-------- gui/src/util/toolCallState.test.ts | 70 ------- 5 files changed, 117 insertions(+), 152 deletions(-) diff --git a/core/tools/definitions/toolDefinitions.test.ts b/core/tools/definitions/toolDefinitions.test.ts index abeac08ef31..265ebe5e12f 100644 --- a/core/tools/definitions/toolDefinitions.test.ts +++ b/core/tools/definitions/toolDefinitions.test.ts @@ -8,6 +8,8 @@ describe("Tool Definitions", () => { rules: [], enableExperimentalTools: false, isSignedIn: false, + isRemote: false, + modelName: "a model", }; // Helper function to get the actual tool object diff --git a/extensions/vscode/src/diff/vertical/handler.ts b/extensions/vscode/src/diff/vertical/handler.ts index 869e5962d76..6312b0f1ac9 100644 --- a/extensions/vscode/src/diff/vertical/handler.ts +++ b/extensions/vscode/src/diff/vertical/handler.ts @@ -343,8 +343,8 @@ export class VerticalDiffHandler implements vscode.Disposable { // Then, we insert our diff lines await this.editor.edit((editBuilder) => { - editBuilder.replace(this.range, replaceContent), - { undoStopAfter: false, undoStopBefore: false }; + (editBuilder.replace(this.range, replaceContent), + { undoStopAfter: false, undoStopBefore: false }); }); // Lastly, we apply decorations diff --git a/gui/src/components/modelSelection/ModelCard.tsx b/gui/src/components/modelSelection/ModelCard.tsx index 2c34bdc51b8..c254b947326 100644 --- a/gui/src/components/modelSelection/ModelCard.tsx +++ b/gui/src/components/modelSelection/ModelCard.tsx @@ -147,7 +147,9 @@ function ModelCard(props: ModelCardProps) {

{props.title}

- {props.tags?.map((tag, i) => )} + {props.tags?.map((tag, i) => ( + + ))}

{props.description}

diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts index fb3c45bbcc1..c61e53e70eb 100644 --- a/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts @@ -18,9 +18,9 @@ describe("singleFindAndReplaceImpl", () => { beforeEach(() => { vi.clearAllMocks(); - + mockResolveRelativePathInDir = vi.mocked(ideUtils.resolveRelativePathInDir); - + mockExtras = { getState: vi.fn(() => ({ config: { @@ -47,7 +47,7 @@ describe("singleFindAndReplaceImpl", () => { }; await expect( - singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), ).rejects.toThrow("filepath is required"); }); @@ -58,7 +58,7 @@ describe("singleFindAndReplaceImpl", () => { }; await expect( - singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), ).rejects.toThrow("old_string is required"); }); @@ -69,7 +69,7 @@ describe("singleFindAndReplaceImpl", () => { }; await expect( - singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), ).rejects.toThrow("new_string is required"); }); @@ -81,7 +81,7 @@ describe("singleFindAndReplaceImpl", () => { }; await expect( - singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), ).rejects.toThrow("old_string and new_string must be different"); }); }); @@ -97,13 +97,15 @@ describe("singleFindAndReplaceImpl", () => { }; await expect( - singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), ).rejects.toThrow("File nonexistent.txt does not exist"); }); it("should resolve relative file paths", async () => { mockResolveRelativePathInDir.mockResolvedValue("/absolute/path/test.txt"); - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test content"); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("test content"); const args = { filepath: "test.txt", @@ -115,10 +117,10 @@ describe("singleFindAndReplaceImpl", () => { expect(mockResolveRelativePathInDir).toHaveBeenCalledWith( "test.txt", - mockExtras.ideMessenger.ide + mockExtras.ideMessenger.ide, ); expect(mockExtras.ideMessenger.ide.readFile).toHaveBeenCalledWith( - "/absolute/path/test.txt" + "/absolute/path/test.txt", ); }); }); @@ -129,7 +131,9 @@ describe("singleFindAndReplaceImpl", () => { }); it("should throw error if old_string is not found in file", async () => { - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("different content"); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("different content"); const args = { filepath: "file.txt", @@ -138,14 +142,14 @@ describe("singleFindAndReplaceImpl", () => { }; await expect( - singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), ).rejects.toThrow("String not found in file: not found"); }); it("should replace single occurrence", async () => { - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( - "Hello world\nThis is a test file\nGoodbye world" - ); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("Hello world\nThis is a test file\nGoodbye world"); const args = { filepath: "file.txt", @@ -155,19 +159,22 @@ describe("singleFindAndReplaceImpl", () => { await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith("applyToFile", { - streamId: "test-uuid", - toolCallId: "tool-call-id", - text: "Hi there\nThis is a test file\nGoodbye world", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }); + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "Hi there\nThis is a test file\nGoodbye world", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }, + ); }); it("should throw error if old_string appears multiple times and replace_all is false", async () => { - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( - "Hello world\nThis is a test file\nGoodbye world" - ); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("Hello world\nThis is a test file\nGoodbye world"); const args = { filepath: "file.txt", @@ -177,16 +184,16 @@ describe("singleFindAndReplaceImpl", () => { }; await expect( - singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), ).rejects.toThrow( - 'String "world" appears 2 times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.' + 'String "world" appears 2 times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.', ); }); it("should replace all occurrences when replace_all is true", async () => { - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( - "Hello world\nThis is a test file\nGoodbye world" - ); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("Hello world\nThis is a test file\nGoodbye world"); const args = { filepath: "file.txt", @@ -197,19 +204,22 @@ describe("singleFindAndReplaceImpl", () => { await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith("applyToFile", { - streamId: "test-uuid", - toolCallId: "tool-call-id", - text: "Hello universe\nThis is a test file\nGoodbye universe", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }); + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "Hello universe\nThis is a test file\nGoodbye universe", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }, + ); }); it("should handle empty new_string (deletion)", async () => { - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( - "Hello world\nThis is a test file" - ); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("Hello world\nThis is a test file"); const args = { filepath: "file.txt", @@ -219,19 +229,24 @@ describe("singleFindAndReplaceImpl", () => { await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith("applyToFile", { - streamId: "test-uuid", - toolCallId: "tool-call-id", - text: "world\nThis is a test file", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }); + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "world\nThis is a test file", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }, + ); }); it("should handle special characters in strings", async () => { - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( - 'const regex = /[a-z]+/g;\nconst text = "Hello $world"' - ); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue( + 'const regex = /[a-z]+/g;\nconst text = "Hello $world"', + ); const args = { filepath: "file.txt", @@ -241,19 +256,24 @@ describe("singleFindAndReplaceImpl", () => { await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith("applyToFile", { - streamId: "test-uuid", - toolCallId: "tool-call-id", - text: 'const regex = /[a-z]+/g;\nconst text = "Hi $universe"', - filepath: "/test/file.txt", - isSearchAndReplace: true, - }); + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: 'const regex = /[a-z]+/g;\nconst text = "Hi $universe"', + filepath: "/test/file.txt", + isSearchAndReplace: true, + }, + ); }); it("should preserve whitespace and indentation", async () => { - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue( - "function test() {\n const value = 'old';\n return value;\n}" - ); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue( + "function test() {\n const value = 'old';\n return value;\n}", + ); const args = { filepath: "file.txt", @@ -263,20 +283,25 @@ describe("singleFindAndReplaceImpl", () => { await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith("applyToFile", { - streamId: "test-uuid", - toolCallId: "tool-call-id", - text: "function test() {\n const value = 'new';\n return value;\n}", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }); + expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + { + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "function test() {\n const value = 'new';\n return value;\n}", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }, + ); }); }); describe("return value", () => { it("should return correct response structure", async () => { mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test content"); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("test content"); const args = { filepath: "file.txt", @@ -284,7 +309,11 @@ describe("singleFindAndReplaceImpl", () => { new_string: "replacement", }; - const result = await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); + const result = await singleFindAndReplaceImpl( + args, + "tool-call-id", + mockExtras, + ); expect(result).toEqual({ respondImmediately: false, @@ -296,9 +325,9 @@ describe("singleFindAndReplaceImpl", () => { describe("error handling", () => { it("should wrap and rethrow errors from readFile", async () => { mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); - mockExtras.ideMessenger.ide.readFile = vi.fn().mockRejectedValue( - new Error("Permission denied") - ); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockRejectedValue(new Error("Permission denied")); const args = { filepath: "file.txt", @@ -307,16 +336,18 @@ describe("singleFindAndReplaceImpl", () => { }; await expect( - singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), ).rejects.toThrow("Failed to apply find and replace: Permission denied"); }); it("should wrap and rethrow errors from applyToFile", async () => { mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test content"); - mockExtras.ideMessenger.request = vi.fn().mockRejectedValue( - new Error("Write failed") - ); + mockExtras.ideMessenger.ide.readFile = vi + .fn() + .mockResolvedValue("test content"); + mockExtras.ideMessenger.request = vi + .fn() + .mockRejectedValue(new Error("Write failed")); const args = { filepath: "file.txt", @@ -325,8 +356,8 @@ describe("singleFindAndReplaceImpl", () => { }; await expect( - singleFindAndReplaceImpl(args, "tool-call-id", mockExtras) + singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), ).rejects.toThrow("Failed to apply find and replace: Write failed"); }); }); -}); \ No newline at end of file +}); diff --git a/gui/src/util/toolCallState.test.ts b/gui/src/util/toolCallState.test.ts index 486f26c666e..b45faabd756 100644 --- a/gui/src/util/toolCallState.test.ts +++ b/gui/src/util/toolCallState.test.ts @@ -480,76 +480,6 @@ describe("addToolCallDeltaToState", () => { expect(currentState.parsedArgs).toEqual({ location: "Paris, France" }); }); - it("should handle arguments streamed in full progressive chunks", () => { - // Test case where model streams args progressively but includes full prefix each time - // e.g. '{"query":"te' -> '{"query":"tes' -> '{"query":"test"}' - const currentState: ToolCallState = { - status: "generating", - toolCall: { - id: "call123", - type: "function", - function: { - name: "searchFiles", - arguments: '{"query":"te', - }, - }, - toolCallId: "call123", - parsedArgs: { query: "te" }, - }; - - const delta: ToolCallDelta = { - function: { - arguments: '{"query":"tes', - }, - }; - - const result = addToolCallDeltaToState(delta, currentState); - expect(result.toolCall.function.arguments).toBe('{"query":"tes'); - expect(result.parsedArgs).toEqual({ query: "tes" }); - - // Continue the streaming - const nextDelta: ToolCallDelta = { - function: { - arguments: '{"query":"test"}', - }, - }; - - const finalResult = addToolCallDeltaToState(nextDelta, result); - expect(finalResult.toolCall.function.arguments).toBe('{"query":"test"}'); - expect(finalResult.parsedArgs).toEqual({ query: "test" }); - }); - - it("should handle when args delta is a superset of current args", () => { - // Test case where model sends a subset of the current arguments - // This can happen in certain LLM implementations - const currentState: ToolCallState = { - status: "generating", - toolCall: { - id: "call123", - type: "function", - function: { - name: "searchFiles", - arguments: '{"query":"test"', - }, - }, - toolCallId: "call123", - parsedArgs: { query: "test" }, - }; - - const delta: ToolCallDelta = { - function: { - arguments: '{"query":"test","limit":10}', - }, - }; - - // The subset should be ignored, keeping the current state - const result = addToolCallDeltaToState(delta, currentState); - expect(result.toolCall.function.arguments).toBe( - '{"query":"test","limit":10}', - ); - expect(result.parsedArgs).toEqual({ query: "test", limit: 10 }); - }); - it("should handle when args are complete JSON and new deltas arrive", () => { // When args are already a valid JSON object, new deltas should not modify it const currentState: ToolCallState = { From 9b0b5814b19a3fcaa630eebba4599fa95fc8dd03 Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Wed, 20 Aug 2025 17:47:35 -0700 Subject: [PATCH 04/14] fix: prettier issues --- extensions/vscode/src/diff/vertical/handler.ts | 4 ++-- gui/src/components/modelSelection/ModelCard.tsx | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/extensions/vscode/src/diff/vertical/handler.ts b/extensions/vscode/src/diff/vertical/handler.ts index 6312b0f1ac9..869e5962d76 100644 --- a/extensions/vscode/src/diff/vertical/handler.ts +++ b/extensions/vscode/src/diff/vertical/handler.ts @@ -343,8 +343,8 @@ export class VerticalDiffHandler implements vscode.Disposable { // Then, we insert our diff lines await this.editor.edit((editBuilder) => { - (editBuilder.replace(this.range, replaceContent), - { undoStopAfter: false, undoStopBefore: false }); + editBuilder.replace(this.range, replaceContent), + { undoStopAfter: false, undoStopBefore: false }; }); // Lastly, we apply decorations diff --git a/gui/src/components/modelSelection/ModelCard.tsx b/gui/src/components/modelSelection/ModelCard.tsx index c254b947326..2c34bdc51b8 100644 --- a/gui/src/components/modelSelection/ModelCard.tsx +++ b/gui/src/components/modelSelection/ModelCard.tsx @@ -147,9 +147,7 @@ function ModelCard(props: ModelCardProps) {

{props.title}

- {props.tags?.map((tag, i) => ( - - ))} + {props.tags?.map((tag, i) => )}

{props.description}

From 832b2d6f463ee5f1a1fa06af6af5b3fd34d4a981 Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Fri, 22 Aug 2025 12:38:10 -0700 Subject: [PATCH 05/14] fix: handle apply error for multi/single edit tools --- gui/src/util/clientTools/multiEditImpl.ts | 25 +++++++++++++------ .../clientTools/singleFindAndReplaceImpl.ts | 25 +++++++++++++------ 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/gui/src/util/clientTools/multiEditImpl.ts b/gui/src/util/clientTools/multiEditImpl.ts index 77b4f867679..eab86992d0a 100644 --- a/gui/src/util/clientTools/multiEditImpl.ts +++ b/gui/src/util/clientTools/multiEditImpl.ts @@ -1,5 +1,6 @@ import { resolveRelativePathInDir } from "core/util/ideUtils"; import { v4 as uuid } from "uuid"; +import { handleEditToolApplyError } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolImpl } from "./callClientTool"; interface EditOperation { @@ -117,13 +118,23 @@ export const multiEditImpl: ClientToolImpl = async ( } // Apply the changes to the file - await extras.ideMessenger.request("applyToFile", { - streamId, - toolCallId, - text: newContent, - filepath: targetFilepath, - isSearchAndReplace: true, - }); + void extras.ideMessenger + .request("applyToFile", { + streamId, + toolCallId, + text: newContent, + filepath: targetFilepath, + isSearchAndReplace: true, + }) + .then((res) => { + if (res.status === "error") { + void extras.dispatch( + handleEditToolApplyError({ + toolCallId, + }), + ); + } + }); // Return success - applyToFile will handle the completion state return { diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts index 3793fec5695..9fd25eea01c 100644 --- a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts @@ -1,5 +1,6 @@ import { resolveRelativePathInDir } from "core/util/ideUtils"; import { v4 as uuid } from "uuid"; +import { handleEditToolApplyError } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolImpl } from "./callClientTool"; export const singleFindAndReplaceImpl: ClientToolImpl = async ( @@ -61,13 +62,23 @@ export const singleFindAndReplaceImpl: ClientToolImpl = async ( } // Apply the changes to the file - await extras.ideMessenger.request("applyToFile", { - streamId, - toolCallId, - text: newContent, - filepath: resolvedFilepath, - isSearchAndReplace: true, - }); + void extras.ideMessenger + .request("applyToFile", { + streamId, + toolCallId, + text: newContent, + filepath: resolvedFilepath, + isSearchAndReplace: true, + }) + .then((res) => { + if (res.status === "error") { + void extras.dispatch( + handleEditToolApplyError({ + toolCallId, + }), + ); + } + }); // Return success - applyToFile will handle the completion state return { From 393c06f97e33815d3c3f5fdbc16c06043dcd0621 Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Fri, 22 Aug 2025 14:42:25 -0700 Subject: [PATCH 06/14] feat: unify edit tool apply handling --- .../thunks/handleApplyStateUpdate.test.ts | 762 ++++++++++++++++++ .../redux/thunks/handleApplyStateUpdate.ts | 33 +- gui/src/util/clientTools/editImpl.ts | 27 +- .../util/clientTools/multiEditImpl.test.ts | 145 ++-- gui/src/util/clientTools/multiEditImpl.ts | 18 +- .../clientTools/searchReplaceImpl.test.ts | 38 +- gui/src/util/clientTools/searchReplaceImpl.ts | 26 +- .../singleFindAndReplaceImpl.test.ts | 121 ++- .../clientTools/singleFindAndReplaceImpl.ts | 18 +- 9 files changed, 924 insertions(+), 264 deletions(-) create mode 100644 gui/src/redux/thunks/handleApplyStateUpdate.test.ts diff --git a/gui/src/redux/thunks/handleApplyStateUpdate.test.ts b/gui/src/redux/thunks/handleApplyStateUpdate.test.ts new file mode 100644 index 00000000000..46ae54c35fc --- /dev/null +++ b/gui/src/redux/thunks/handleApplyStateUpdate.test.ts @@ -0,0 +1,762 @@ +import { ApplyState, ApplyToFilePayload, ToolCallState } from "core"; +import { EDIT_MODE_STREAM_ID } from "core/edit/constants"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { logAgentModeEditOutcome } from "../../util/editOutcomeLogger"; +import { + selectApplyStateByToolCallId, + selectToolCallById, +} from "../selectors/selectToolCalls"; +import { updateEditStateApplyState } from "../slices/editState"; +import { + acceptToolCall, + errorToolCall, + updateApplyState, + updateToolCallOutput, +} from "../slices/sessionSlice"; +import { findToolCallById, logToolUsage } from "../util"; +import { exitEdit } from "./edit"; +import { + applyForEditTool, + handleApplyStateUpdate, +} from "./handleApplyStateUpdate"; +import { streamResponseAfterToolCall } from "./streamResponseAfterToolCall"; + +// Mock dependencies +vi.mock("../../util/editOutcomeLogger", () => ({ + logAgentModeEditOutcome: vi.fn(), +})); + +vi.mock("../selectors/selectToolCalls", () => ({ + selectApplyStateByToolCallId: vi.fn(), + selectToolCallById: vi.fn(), +})); + +vi.mock("../slices/editState", () => ({ + updateEditStateApplyState: vi.fn(() => ({ + type: "editState/updateApplyState", + })), +})); + +vi.mock("../slices/sessionSlice", () => ({ + acceptToolCall: vi.fn(() => ({ type: "session/acceptToolCall" })), + errorToolCall: vi.fn(() => ({ type: "session/errorToolCall" })), + updateApplyState: vi.fn(() => ({ type: "session/updateApplyState" })), + updateToolCallOutput: vi.fn(() => ({ type: "session/updateToolCallOutput" })), +})); + +vi.mock("../util", () => ({ + findToolCallById: vi.fn(), + logToolUsage: vi.fn(), +})); + +vi.mock("./edit", () => ({ + exitEdit: vi.fn(() => ({ type: "edit/exitEdit" })), +})); + +vi.mock("./streamResponseAfterToolCall", () => ({ + streamResponseAfterToolCall: vi.fn(() => ({ + type: "session/streamResponseAfterToolCall", + })), +})); + +const UNUSED_TOOL_CALL_PARAMS = { + toolCall: { + id: "name", + function: { + name: "unused", + arguments: "unused", + }, + type: "function" as const, + }, + parsedArgs: {}, +}; + +describe("handleApplyStateUpdate", () => { + let mockDispatch: any; + let mockGetState: any; + let mockExtra: any; + + beforeEach(() => { + vi.clearAllMocks(); + mockDispatch = vi.fn(); + mockGetState = vi.fn(); + mockExtra = { + ideMessenger: { + post: vi.fn(), + request: vi.fn(), + }, + }; + }); + + describe("edit mode handling", () => { + it("should handle edit mode apply state updates", async () => { + const applyState: ApplyState = { + streamId: EDIT_MODE_STREAM_ID, + toolCallId: "test-tool-call", + status: "streaming", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(updateEditStateApplyState).toHaveBeenCalledWith(applyState); + expect(mockDispatch).toHaveBeenCalledWith( + expect.objectContaining({ type: "editState/updateApplyState" }), + ); + }); + + it("should exit edit mode when status is closed", async () => { + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "done", + ...UNUSED_TOOL_CALL_PARAMS, + }; + vi.mocked(findToolCallById).mockReturnValue(toolCallState); + + mockGetState.mockReturnValue({ + session: { history: [] }, + }); + + const applyState: ApplyState = { + streamId: EDIT_MODE_STREAM_ID, + toolCallId: "test-tool-call", + status: "closed", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(logToolUsage).toHaveBeenCalledWith( + toolCallState, + true, + true, + mockExtra.ideMessenger, + ); + expect(exitEdit).toHaveBeenCalledWith({}); + expect(mockDispatch).toHaveBeenCalledWith( + expect.objectContaining({ type: "edit/exitEdit" }), + ); + }); + }); + + describe("chat/agent mode handling", () => { + it("should handle non-edit mode apply state updates", async () => { + const applyState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status: "streaming", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(updateApplyState).toHaveBeenCalledWith(applyState); + expect(mockDispatch).toHaveBeenCalledWith( + expect.objectContaining({ type: "session/updateApplyState" }), + ); + }); + + it("should auto-accept diffs when configured and status is done", async () => { + mockGetState.mockReturnValue({ + config: { + config: { + ui: { + autoAcceptEditToolDiffs: true, + }, + }, + }, + session: { history: [] }, + }); + + const applyState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status: "done", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(mockExtra.ideMessenger.post).toHaveBeenCalledWith("acceptDiff", { + streamId: "chat-stream", + filepath: "test.txt", + }); + }); + + it("should not auto-accept when config is disabled", async () => { + mockGetState.mockReturnValue({ + config: { + config: { + ui: { + autoAcceptEditToolDiffs: false, + }, + }, + }, + session: { history: [] }, + }); + + const applyState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status: "done", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(mockExtra.ideMessenger.post).not.toHaveBeenCalled(); + }); + }); + + describe("closed status handling", () => { + it("should handle accepted tool call closure", async () => { + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "done", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const newApplyState = { streamId: "chat-stream" }; + + vi.mocked(findToolCallById).mockReturnValue(toolCallState); + mockGetState.mockReturnValue({ + session: { + history: [], + codeBlockApplyStates: { + states: [newApplyState], + }, + }, + config: { config: {} }, + }); + + const applyState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status: "closed", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(logToolUsage).toHaveBeenCalledWith( + toolCallState, + true, + true, + mockExtra.ideMessenger, + ); + expect(logAgentModeEditOutcome).toHaveBeenCalledWith( + [], + {}, + toolCallState, + newApplyState, + true, + mockExtra.ideMessenger, + ); + expect(acceptToolCall).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + }); + expect(streamResponseAfterToolCall).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + }); + }); + + it("should handle canceled tool call closure", async () => { + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "canceled", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const newApplyState = { streamId: "chat-stream" }; + + vi.mocked(findToolCallById).mockReturnValue(toolCallState); + mockGetState.mockReturnValue({ + session: { + history: [], + codeBlockApplyStates: { + states: [newApplyState], + }, + }, + config: { config: {} }, + }); + + const applyState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status: "closed", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(logToolUsage).toHaveBeenCalledWith( + toolCallState, + false, + true, + mockExtra.ideMessenger, + ); + expect(logAgentModeEditOutcome).toHaveBeenCalledWith( + [], + {}, + toolCallState, + newApplyState, + false, + mockExtra.ideMessenger, + ); + expect(acceptToolCall).not.toHaveBeenCalled(); + expect(streamResponseAfterToolCall).not.toHaveBeenCalled(); + }); + + it("should handle errored tool call closure", async () => { + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "errored", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const newApplyState = { streamId: "chat-stream" }; + + vi.mocked(findToolCallById).mockReturnValue(toolCallState); + mockGetState.mockReturnValue({ + session: { + history: [], + codeBlockApplyStates: { + states: [newApplyState], + }, + }, + config: { config: {} }, + }); + + const applyState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status: "closed", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(acceptToolCall).not.toHaveBeenCalled(); + expect(streamResponseAfterToolCall).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + }); + }); + + it("should handle closure when no tool call found", async () => { + vi.mocked(findToolCallById).mockReturnValue(undefined); + mockGetState.mockReturnValue({ + session: { + history: [], + codeBlockApplyStates: { + states: [], + }, + }, + }); + + const applyState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status: "closed", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(logToolUsage).not.toHaveBeenCalled(); + expect(acceptToolCall).not.toHaveBeenCalled(); + expect(streamResponseAfterToolCall).not.toHaveBeenCalled(); + }); + + it("should handle closure when no apply state found", async () => { + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "done", + ...UNUSED_TOOL_CALL_PARAMS, + }; + + vi.mocked(findToolCallById).mockReturnValue(toolCallState); + mockGetState.mockReturnValue({ + session: { + history: [], + codeBlockApplyStates: { + states: [], + }, + }, + config: { config: {} }, + }); + + const applyState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status: "closed", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(logAgentModeEditOutcome).not.toHaveBeenCalled(); + expect(acceptToolCall).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + }); + }); + }); + + describe("edge cases", () => { + it("should handle apply state without toolCallId", async () => { + const applyState: ApplyState = { + streamId: "chat-stream", + status: "streaming", + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(updateApplyState).toHaveBeenCalledWith(applyState); + expect(acceptToolCall).not.toHaveBeenCalled(); + expect(streamResponseAfterToolCall).not.toHaveBeenCalled(); + }); + + it("should handle different status values", async () => { + const statusValues: ApplyState["status"][] = [ + "not-started", + "streaming", + "done", + "closed", + ]; + + for (const status of statusValues) { + vi.clearAllMocks(); + + const applyState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status, + filepath: "test.txt", + numDiffs: 1, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(updateApplyState).toHaveBeenCalledWith(applyState); + } + }); + }); +}); + +describe("applyForEditTool", () => { + let mockDispatch: any; + let mockGetState: any; + let mockExtra: any; + + beforeEach(() => { + vi.clearAllMocks(); + mockDispatch = vi.fn(); + mockGetState = vi.fn(); + mockExtra = { + ideMessenger: { + request: vi.fn(), + }, + }; + }); + + describe("successful application", () => { + it("should successfully apply changes to file", async () => { + const payload: ApplyToFilePayload & { toolCallId: string } = { + toolCallId: "test-tool-call", + streamId: "test-stream", + filepath: "test.txt", + text: "new content", + isSearchAndReplace: true, + }; + + mockExtra.ideMessenger.request.mockResolvedValue({ status: "success" }); + + const thunk = applyForEditTool(payload); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(updateApplyState).toHaveBeenCalledWith({ + streamId: "test-stream", + toolCallId: "test-tool-call", + status: "not-started", + }); + expect(mockDispatch).toHaveBeenCalledWith( + expect.objectContaining({ type: "session/updateApplyState" }), + ); + expect(mockExtra.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + payload, + ); + }); + }); + + describe("error handling", () => { + it("should handle IDE messenger request failure", async () => { + const payload: ApplyToFilePayload & { toolCallId: string } = { + toolCallId: "test-tool-call", + streamId: "test-stream", + filepath: "test.txt", + text: "new content", + isSearchAndReplace: true, + }; + + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "calling", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const applyState: ApplyState = { + status: "streaming", + streamId: "test-stream", + }; + + vi.mocked(selectToolCallById).mockReturnValue(toolCallState); + vi.mocked(selectApplyStateByToolCallId).mockReturnValue(applyState); + mockGetState.mockReturnValue({}); + + mockExtra.ideMessenger.request.mockRejectedValue( + new Error("Request failed"), + ); + + const thunk = applyForEditTool(payload); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(errorToolCall).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + }); + expect(updateToolCallOutput).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + contextItems: [ + { + icon: "problems", + name: "Apply Error", + description: "Failed to apply changes", + content: `Error editing file: failed to apply changes to file.\n\nPlease try again with correct args or notify the user and request further instructions.`, + hidden: false, + }, + ], + }); + }); + + it("should handle IDE messenger error response", async () => { + const payload: ApplyToFilePayload & { toolCallId: string } = { + toolCallId: "test-tool-call", + streamId: "test-stream", + filepath: "test.txt", + text: "new content", + isSearchAndReplace: true, + }; + + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "calling", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const applyState: ApplyState = { + status: "streaming", + streamId: "test-stream", + }; + + vi.mocked(selectToolCallById).mockReturnValue(toolCallState); + vi.mocked(selectApplyStateByToolCallId).mockReturnValue(applyState); + mockGetState.mockReturnValue({}); + + mockExtra.ideMessenger.request.mockResolvedValue({ status: "error" }); + + const thunk = applyForEditTool(payload); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(errorToolCall).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + }); + expect(updateToolCallOutput).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + contextItems: [ + { + icon: "problems", + name: "Apply Error", + description: "Failed to apply changes", + content: `Error editing file: failed to apply changes to file.\n\nPlease try again with correct args or notify the user and request further instructions.`, + hidden: false, + }, + ], + }); + }); + + it("should not error if tool call is not in calling state", async () => { + const payload: ApplyToFilePayload & { toolCallId: string } = { + toolCallId: "test-tool-call", + streamId: "test-stream", + filepath: "test.txt", + text: "new content", + isSearchAndReplace: true, + }; + + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "done", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const applyState: ApplyState = { + status: "streaming", + streamId: "test-stream", + }; + + vi.mocked(selectToolCallById).mockReturnValue(toolCallState); + vi.mocked(selectApplyStateByToolCallId).mockReturnValue(applyState); + mockGetState.mockReturnValue({}); + + mockExtra.ideMessenger.request.mockRejectedValue( + new Error("Request failed"), + ); + + const thunk = applyForEditTool(payload); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(errorToolCall).not.toHaveBeenCalled(); + expect(updateToolCallOutput).not.toHaveBeenCalled(); + }); + + it("should not error if apply state is closed", async () => { + const payload: ApplyToFilePayload & { toolCallId: string } = { + toolCallId: "test-tool-call", + streamId: "test-stream", + filepath: "test.txt", + text: "new content", + isSearchAndReplace: true, + }; + + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "calling", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const applyState: ApplyState = { + status: "closed", + streamId: "test-stream", + }; // Already closed + + vi.mocked(selectToolCallById).mockReturnValue(toolCallState); + vi.mocked(selectApplyStateByToolCallId).mockReturnValue(applyState); + mockGetState.mockReturnValue({}); + + mockExtra.ideMessenger.request.mockRejectedValue( + new Error("Request failed"), + ); + + const thunk = applyForEditTool(payload); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(errorToolCall).not.toHaveBeenCalled(); + expect(updateToolCallOutput).not.toHaveBeenCalled(); + }); + + it("should not error if tool call state is not found", async () => { + const payload: ApplyToFilePayload & { toolCallId: string } = { + toolCallId: "test-tool-call", + streamId: "test-stream", + filepath: "test.txt", + text: "new content", + isSearchAndReplace: true, + }; + + vi.mocked(selectToolCallById).mockReturnValue(undefined); // Tool call not found + vi.mocked(selectApplyStateByToolCallId).mockReturnValue({ + status: "streaming", + streamId: "test-stream", + }); + mockGetState.mockReturnValue({}); + + mockExtra.ideMessenger.request.mockRejectedValue( + new Error("Request failed"), + ); + + const thunk = applyForEditTool(payload); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(errorToolCall).not.toHaveBeenCalled(); + expect(updateToolCallOutput).not.toHaveBeenCalled(); + }); + + it("should not error if apply state is not found", async () => { + const payload: ApplyToFilePayload & { toolCallId: string } = { + toolCallId: "test-tool-call", + streamId: "test-stream", + filepath: "test.txt", + text: "new content", + isSearchAndReplace: true, + }; + + vi.mocked(selectApplyStateByToolCallId).mockReturnValue(undefined); // Apply state not found + mockGetState.mockReturnValue({}); + + mockExtra.ideMessenger.request.mockRejectedValue( + new Error("Request failed"), + ); + + const thunk = applyForEditTool(payload); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(errorToolCall).not.toHaveBeenCalled(); + expect(updateToolCallOutput).not.toHaveBeenCalled(); + }); + }); + + describe("edge cases", () => { + it("should handle different payload types", async () => { + const payloads: (ApplyToFilePayload & { toolCallId: string })[] = [ + { + toolCallId: "test-1", + streamId: "stream-1", + filepath: "file1.txt", + text: "content 1", + isSearchAndReplace: false, + }, + { + toolCallId: "test-2", + streamId: "stream-2", + filepath: "file2.js", + text: "content 2", + isSearchAndReplace: true, + }, + ]; + + for (const payload of payloads) { + vi.clearAllMocks(); + mockExtra.ideMessenger.request.mockResolvedValue({ status: "success" }); + + const thunk = applyForEditTool(payload); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(mockExtra.ideMessenger.request).toHaveBeenCalledWith( + "applyToFile", + payload, + ); + expect(updateApplyState).toHaveBeenCalledWith({ + streamId: payload.streamId, + toolCallId: payload.toolCallId, + status: "not-started", + }); + } + }); + }); +}); diff --git a/gui/src/redux/thunks/handleApplyStateUpdate.ts b/gui/src/redux/thunks/handleApplyStateUpdate.ts index 1a56fe04be1..25532e75079 100644 --- a/gui/src/redux/thunks/handleApplyStateUpdate.ts +++ b/gui/src/redux/thunks/handleApplyStateUpdate.ts @@ -1,5 +1,5 @@ import { createAsyncThunk } from "@reduxjs/toolkit"; -import { ApplyState } from "core"; +import { ApplyState, ApplyToFilePayload } from "core"; import { EDIT_MODE_STREAM_ID } from "core/edit/constants"; import { logAgentModeEditOutcome } from "../../util/editOutcomeLogger"; import { @@ -105,13 +105,30 @@ export const handleApplyStateUpdate = createAsyncThunk< }, ); -export const handleEditToolApplyError = createAsyncThunk< +export const applyForEditTool = createAsyncThunk< void, - { toolCallId: string }, + ApplyToFilePayload & { toolCallId: string }, ThunkApiType ->( - "apply/handleEditError", - async ({ toolCallId }, { dispatch, getState, extra }) => { +>("apply/editTool", async (payload, { dispatch, getState, extra }) => { + const { toolCallId, streamId } = payload; + dispatch( + updateApplyState({ + streamId, + toolCallId, + status: "not-started", + }), + ); + + let didError = false; + try { + const response = await extra.ideMessenger.request("applyToFile", payload); + if (response.status === "error") { + didError = true; + } + } catch (e) { + didError = true; + } + if (didError) { const state = getState(); const toolCallState = selectToolCallById(state, toolCallId); @@ -149,5 +166,5 @@ export const handleEditToolApplyError = createAsyncThunk< }), ); } - }, -); + } +}); diff --git a/gui/src/util/clientTools/editImpl.ts b/gui/src/util/clientTools/editImpl.ts index edd49dd0617..4dec61a65c3 100644 --- a/gui/src/util/clientTools/editImpl.ts +++ b/gui/src/util/clientTools/editImpl.ts @@ -1,7 +1,6 @@ import { resolveRelativePathInDir } from "core/util/ideUtils"; import { v4 as uuid } from "uuid"; -import { updateApplyState } from "../../redux/slices/sessionSlice"; -import { handleEditToolApplyError } from "../../redux/thunks/handleApplyStateUpdate"; +import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolImpl } from "./callClientTool"; export const editToolImpl: ClientToolImpl = async ( args, @@ -21,29 +20,15 @@ export const editToolImpl: ClientToolImpl = async ( throw new Error(`${args.filepath} does not exist`); } const streamId = uuid(); - extras.dispatch( - updateApplyState({ - streamId, - toolCallId, - status: "not-started", - }), - ); - void extras.ideMessenger - .request("applyToFile", { + void extras.dispatch( + applyForEditTool({ streamId, text: args.changes, toolCallId, filepath: firstUriMatch, - }) - .then((res) => { - if (res.status === "error") { - void extras.dispatch( - handleEditToolApplyError({ - toolCallId, - }), - ); - } - }); + }), + ); + return { respondImmediately: false, output: undefined, // No immediate output. diff --git a/gui/src/util/clientTools/multiEditImpl.test.ts b/gui/src/util/clientTools/multiEditImpl.test.ts index edbba81fe99..f8069bff130 100644 --- a/gui/src/util/clientTools/multiEditImpl.test.ts +++ b/gui/src/util/clientTools/multiEditImpl.test.ts @@ -1,5 +1,6 @@ import * as ideUtils from "core/util/ideUtils"; import { beforeEach, describe, expect, it, Mock, vi } from "vitest"; +import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolExtras } from "./callClientTool"; import { multiEditImpl } from "./multiEditImpl"; @@ -11,14 +12,20 @@ vi.mock("core/util/ideUtils", () => ({ resolveRelativePathInDir: vi.fn(), })); +vi.mock("../../redux/thunks/handleApplyStateUpdate", () => ({ + applyForEditTool: vi.fn(), +})); + describe("multiEditImpl", () => { let mockExtras: ClientToolExtras; let mockResolveRelativePathInDir: Mock; + let mockApplyForEditTool: Mock; beforeEach(() => { vi.clearAllMocks(); mockResolveRelativePathInDir = vi.mocked(ideUtils.resolveRelativePathInDir); + mockApplyForEditTool = vi.mocked(applyForEditTool); mockExtras = { getState: vi.fn(() => ({ @@ -106,16 +113,13 @@ describe("multiEditImpl", () => { mockExtras, ); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "id", - text: "Hi world", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "id", + text: "Hi world", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); }); it("should apply multiple edits sequentially", async () => { @@ -135,16 +139,13 @@ describe("multiEditImpl", () => { mockExtras, ); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "id", - text: "Hi universe\nGoodbye universe", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "id", + text: "Hi universe\nGoodbye universe", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); }); it("should handle edits that depend on previous edits", async () => { @@ -164,16 +165,13 @@ describe("multiEditImpl", () => { mockExtras, ); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "id", - text: "let x = 2;", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "id", + text: "let x = 2;", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); }); it("should throw if string not found in edit sequence", async () => { @@ -227,16 +225,13 @@ describe("multiEditImpl", () => { mockExtras, ); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "id", - text: "New content\nLine 2", - filepath: "new.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "id", + text: "New content\nLine 2", + filepath: "new.txt", + isSearchAndReplace: true, + }); }); it("should create and edit new file", async () => { @@ -254,16 +249,13 @@ describe("multiEditImpl", () => { mockExtras, ); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "id", - text: "Hello universe", - filepath: "new.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "id", + text: "Hello universe", + filepath: "new.txt", + isSearchAndReplace: true, + }); }); }); @@ -286,16 +278,13 @@ describe("multiEditImpl", () => { mockExtras, ); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "id", - text: "qux bar qux baz qux", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "id", + text: "qux bar qux baz qux", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); }); it("should handle mixed replace_all settings", async () => { @@ -315,16 +304,13 @@ describe("multiEditImpl", () => { mockExtras, ); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "id", - text: "a b a z a", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "id", + text: "a b a z a", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); }); }); @@ -346,25 +332,6 @@ describe("multiEditImpl", () => { ), ).rejects.toThrow("Failed to apply multi edit: Read failed"); }); - - it("should wrap applyToFile errors", async () => { - mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); - mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test"); - mockExtras.ideMessenger.request = vi - .fn() - .mockRejectedValue(new Error("Write failed")); - - await expect( - multiEditImpl( - { - filepath: "file.txt", - edits: [{ old_string: "test", new_string: "new" }], - }, - "id", - mockExtras, - ), - ).rejects.toThrow("Failed to apply multi edit: Write failed"); - }); }); describe("return value", () => { diff --git a/gui/src/util/clientTools/multiEditImpl.ts b/gui/src/util/clientTools/multiEditImpl.ts index eab86992d0a..abe306a419f 100644 --- a/gui/src/util/clientTools/multiEditImpl.ts +++ b/gui/src/util/clientTools/multiEditImpl.ts @@ -1,6 +1,6 @@ import { resolveRelativePathInDir } from "core/util/ideUtils"; import { v4 as uuid } from "uuid"; -import { handleEditToolApplyError } from "../../redux/thunks/handleApplyStateUpdate"; +import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolImpl } from "./callClientTool"; interface EditOperation { @@ -118,23 +118,15 @@ export const multiEditImpl: ClientToolImpl = async ( } // Apply the changes to the file - void extras.ideMessenger - .request("applyToFile", { + void extras.dispatch( + applyForEditTool({ streamId, toolCallId, text: newContent, filepath: targetFilepath, isSearchAndReplace: true, - }) - .then((res) => { - if (res.status === "error") { - void extras.dispatch( - handleEditToolApplyError({ - toolCallId, - }), - ); - } - }); + }), + ); // Return success - applyToFile will handle the completion state return { diff --git a/gui/src/util/clientTools/searchReplaceImpl.test.ts b/gui/src/util/clientTools/searchReplaceImpl.test.ts index 8d8e2ed07fc..69423a13033 100644 --- a/gui/src/util/clientTools/searchReplaceImpl.test.ts +++ b/gui/src/util/clientTools/searchReplaceImpl.test.ts @@ -2,6 +2,7 @@ import { findSearchMatch } from "core/edit/searchAndReplace/findSearchMatch"; import { parseAllSearchReplaceBlocks } from "core/edit/searchAndReplace/parseSearchReplaceBlock"; import { resolveRelativePathInDir } from "core/util/ideUtils"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolExtras } from "./callClientTool"; import { searchReplaceToolImpl } from "./searchReplaceImpl"; @@ -12,10 +13,14 @@ vi.mock("core/util/ideUtils"); vi.mock("uuid", () => ({ v4: vi.fn(() => "test-stream-id"), })); +vi.mock("../../redux/thunks/handleApplyStateUpdate", () => ({ + applyForEditTool: vi.fn(), +})); const mockFindSearchMatch = vi.mocked(findSearchMatch); const mockParseAllSearchReplaceBlocks = vi.mocked(parseAllSearchReplaceBlocks); const mockResolveRelativePathInDir = vi.mocked(resolveRelativePathInDir); +const mockApplyForEditTool = vi.mocked(applyForEditTool); describe("searchReplaceToolImpl", () => { let mockExtras: ClientToolExtras; @@ -144,8 +149,6 @@ describe("searchReplaceToolImpl", () => { endIndex, strategyName: "exactMatch", }); - mockIdeMessenger.request.mockResolvedValue({ status: "success" }); - const result = await searchReplaceToolImpl( { filepath: "test.js", diffs: ["mock diff content"] }, "tool-call-id", @@ -159,7 +162,7 @@ describe("searchReplaceToolImpl", () => { }); // Verify applyToFile was called with correct parameters - expect(mockIdeMessenger.request).toHaveBeenCalledWith("applyToFile", { + expect(mockApplyForEditTool).toHaveBeenCalledWith({ text: expectedFinalContent, streamId: "test-stream-id", filepath: "/resolved/path/test.js", @@ -202,8 +205,6 @@ describe("searchReplaceToolImpl", () => { endIndex, strategyName: "exactMatch", }); - mockIdeMessenger.request.mockResolvedValue({ status: "success" }); - const result = await searchReplaceToolImpl( { filepath: "test.js", diffs: ["mock diff content"] }, "tool-call-id", @@ -217,7 +218,7 @@ describe("searchReplaceToolImpl", () => { }); // Verify applyToFile was called with correct parameters - expect(mockIdeMessenger.request).toHaveBeenCalledWith("applyToFile", { + expect(mockApplyForEditTool).toHaveBeenCalledWith({ text: expectedFinalContent, streamId: "test-stream-id", filepath: "/resolved/path/test.js", @@ -283,8 +284,6 @@ const c = 3;`; strategyName: "exactMatch", }); - mockIdeMessenger.request.mockResolvedValue({ status: "success" }); - const result = await searchReplaceToolImpl( { filepath: "test.js", diffs: ["mock diff content"] }, "tool-call-id", @@ -310,7 +309,7 @@ const c = 3;`; "const b = 2;", ); // Verify final applyToFile call - expect(mockIdeMessenger.request).toHaveBeenCalledWith("applyToFile", { + expect(mockApplyForEditTool).toHaveBeenCalledWith({ text: expectedFinalContent, streamId: "test-stream-id", filepath: "/resolved/path/test.js", @@ -379,8 +378,6 @@ const c = 3;`; strategyName: "exactMatch", }); - mockIdeMessenger.request.mockResolvedValue({ status: "success" }); - const result = await searchReplaceToolImpl( { filepath: "test.js", diffs: ["first diff", "second diff"] }, "tool-call-id", @@ -405,7 +402,7 @@ const c = 3;`; ); // Verify final applyToFile call - expect(mockIdeMessenger.request).toHaveBeenCalledWith("applyToFile", { + expect(mockApplyForEditTool).toHaveBeenCalledWith({ text: expectedFinalContent, streamId: "test-stream-id", filepath: "/resolved/path/test.js", @@ -440,7 +437,6 @@ keep this too`; endIndex: 26, // End of "remove this line" strategyName: "exactMatch", }); - mockIdeMessenger.request.mockResolvedValue({ status: "success" }); const result = await searchReplaceToolImpl( { filepath: "test.txt", diffs: ["mock diff content"] }, @@ -453,7 +449,7 @@ keep this too`; output: undefined, }); - expect(mockIdeMessenger.request).toHaveBeenCalledWith("applyToFile", { + expect(mockApplyForEditTool).toHaveBeenCalledWith({ text: expectedFinalContent, streamId: "test-stream-id", filepath: "/resolved/path/test.txt", @@ -480,15 +476,13 @@ keep this too`; endIndex: 11, strategyName: "exactMatch", }); - mockIdeMessenger.request.mockResolvedValue({ status: "success" }); - await searchReplaceToolImpl( { filepath: "test.txt", diffs: ["mock diff content"] }, "tool-call-id", mockExtras, ); - expect(mockIdeMessenger.request).toHaveBeenCalledWith("applyToFile", { + expect(mockApplyForEditTool).toHaveBeenCalledWith({ text: expectedFinalContent, streamId: "test-stream-id", filepath: "/resolved/path/test.txt", @@ -602,15 +596,13 @@ keep this too`; endIndex: 0, strategyName: "exactMatch", }); - mockIdeMessenger.request.mockResolvedValue({ status: "success" }); - await searchReplaceToolImpl( { filepath: "test.txt", diffs: ["mock diff content"] }, "tool-call-id", mockExtras, ); - expect(mockIdeMessenger.request).toHaveBeenCalledWith("applyToFile", { + expect(mockApplyForEditTool).toHaveBeenCalledWith({ text: expectedFinalContent, streamId: "test-stream-id", filepath: "/resolved/path/test.txt", @@ -637,7 +629,6 @@ keep this too`; endIndex: originalContent.length, strategyName: "exactMatch", }); - mockIdeMessenger.request.mockResolvedValue({ status: "success" }); await searchReplaceToolImpl( { filepath: "test.txt", diffs: ["mock diff content"] }, @@ -645,7 +636,7 @@ keep this too`; mockExtras, ); - expect(mockIdeMessenger.request).toHaveBeenCalledWith("applyToFile", { + expect(mockApplyForEditTool).toHaveBeenCalledWith({ text: expectedFinalContent, streamId: "test-stream-id", filepath: "/resolved/path/test.txt", @@ -673,7 +664,6 @@ keep this too`; endIndex: 4, strategyName: "exactMatch", }); - mockIdeMessenger.request.mockResolvedValue({ status: "success" }); await searchReplaceToolImpl( { filepath: "relative/test.txt", diffs: ["mock diff content"] }, @@ -693,7 +683,7 @@ keep this too`; "/resolved/path/test.txt", ); expect(mockFindSearchMatch).toHaveBeenCalledWith(originalContent, "test"); - expect(mockIdeMessenger.request).toHaveBeenCalledWith("applyToFile", { + expect(mockApplyForEditTool).toHaveBeenCalledWith({ text: "updated content", streamId: "test-stream-id", filepath: "/resolved/path/test.txt", diff --git a/gui/src/util/clientTools/searchReplaceImpl.ts b/gui/src/util/clientTools/searchReplaceImpl.ts index b734b61ca02..ab931d40a28 100644 --- a/gui/src/util/clientTools/searchReplaceImpl.ts +++ b/gui/src/util/clientTools/searchReplaceImpl.ts @@ -3,8 +3,7 @@ import { parseAllSearchReplaceBlocks } from "core/edit/searchAndReplace/parseSea import { resolveRelativePathInDir } from "core/util/ideUtils"; import posthog from "posthog-js"; import { v4 as uuid } from "uuid"; -import { updateApplyState } from "../../redux/slices/sessionSlice"; -import { handleEditToolApplyError } from "../../redux/thunks/handleApplyStateUpdate"; +import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolImpl } from "./callClientTool"; export const searchReplaceToolImpl: ClientToolImpl = async ( @@ -84,30 +83,15 @@ export const searchReplaceToolImpl: ClientToolImpl = async ( // This works becaues of our logic in `applyCodeBlock` that determines // that the full file rewrite here can be applied instantly, so the diff // lines are generated with meyers diff and streamed instantaneously - extras.dispatch( - updateApplyState({ - streamId, - toolCallId, - status: "not-started", - }), - ); - void extras.ideMessenger - .request("applyToFile", { + void extras.dispatch( + applyForEditTool({ streamId, toolCallId, text: currentContent, filepath: resolvedFilepath, isSearchAndReplace: true, - }) - .then((res) => { - if (res.status === "error") { - void extras.dispatch( - handleEditToolApplyError({ - toolCallId, - }), - ); - } - }); + }), + ); // Return success - applyToFile will handle the completion state return { diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts index c61e53e70eb..0b380dc1880 100644 --- a/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts @@ -1,9 +1,8 @@ -import { describe, it, expect, beforeEach, vi, Mock } from "vitest"; -import { v4 as uuid } from "uuid"; -import { singleFindAndReplaceImpl } from "./singleFindAndReplaceImpl"; -import { ClientToolExtras } from "./callClientTool"; import * as ideUtils from "core/util/ideUtils"; - +import { beforeEach, describe, expect, it, Mock, vi } from "vitest"; +import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; +import { ClientToolExtras } from "./callClientTool"; +import { singleFindAndReplaceImpl } from "./singleFindAndReplaceImpl"; vi.mock("uuid", () => ({ v4: vi.fn(() => "test-uuid"), })); @@ -12,14 +11,20 @@ vi.mock("core/util/ideUtils", () => ({ resolveRelativePathInDir: vi.fn(), })); +vi.mock("../../redux/thunks/handleApplyStateUpdate", () => ({ + applyForEditTool: vi.fn(), +})); + describe("singleFindAndReplaceImpl", () => { let mockExtras: ClientToolExtras; let mockResolveRelativePathInDir: Mock; + let mockApplyForEditTool: Mock; beforeEach(() => { vi.clearAllMocks(); mockResolveRelativePathInDir = vi.mocked(ideUtils.resolveRelativePathInDir); + mockApplyForEditTool = vi.mocked(applyForEditTool); mockExtras = { getState: vi.fn(() => ({ @@ -159,16 +164,14 @@ describe("singleFindAndReplaceImpl", () => { await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "tool-call-id", - text: "Hi there\nThis is a test file\nGoodbye world", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }, - ); + // Check that the dispatch was called with the applyForEditTool thunk + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "Hi there\nThis is a test file\nGoodbye world", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); }); it("should throw error if old_string appears multiple times and replace_all is false", async () => { @@ -204,16 +207,13 @@ describe("singleFindAndReplaceImpl", () => { await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "tool-call-id", - text: "Hello universe\nThis is a test file\nGoodbye universe", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "Hello universe\nThis is a test file\nGoodbye universe", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); }); it("should handle empty new_string (deletion)", async () => { @@ -229,16 +229,13 @@ describe("singleFindAndReplaceImpl", () => { await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "tool-call-id", - text: "world\nThis is a test file", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "world\nThis is a test file", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); }); it("should handle special characters in strings", async () => { @@ -256,16 +253,13 @@ describe("singleFindAndReplaceImpl", () => { await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "tool-call-id", - text: 'const regex = /[a-z]+/g;\nconst text = "Hi $universe"', - filepath: "/test/file.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: 'const regex = /[a-z]+/g;\nconst text = "Hi $universe"', + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); }); it("should preserve whitespace and indentation", async () => { @@ -283,16 +277,13 @@ describe("singleFindAndReplaceImpl", () => { await singleFindAndReplaceImpl(args, "tool-call-id", mockExtras); - expect(mockExtras.ideMessenger.request).toHaveBeenCalledWith( - "applyToFile", - { - streamId: "test-uuid", - toolCallId: "tool-call-id", - text: "function test() {\n const value = 'new';\n return value;\n}", - filepath: "/test/file.txt", - isSearchAndReplace: true, - }, - ); + expect(mockApplyForEditTool).toHaveBeenCalledWith({ + streamId: "test-uuid", + toolCallId: "tool-call-id", + text: "function test() {\n const value = 'new';\n return value;\n}", + filepath: "/test/file.txt", + isSearchAndReplace: true, + }); }); }); @@ -339,25 +330,5 @@ describe("singleFindAndReplaceImpl", () => { singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), ).rejects.toThrow("Failed to apply find and replace: Permission denied"); }); - - it("should wrap and rethrow errors from applyToFile", async () => { - mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); - mockExtras.ideMessenger.ide.readFile = vi - .fn() - .mockResolvedValue("test content"); - mockExtras.ideMessenger.request = vi - .fn() - .mockRejectedValue(new Error("Write failed")); - - const args = { - filepath: "file.txt", - old_string: "test", - new_string: "replacement", - }; - - await expect( - singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), - ).rejects.toThrow("Failed to apply find and replace: Write failed"); - }); }); }); diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts index 9fd25eea01c..b9ba5a9bb60 100644 --- a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts @@ -1,6 +1,6 @@ import { resolveRelativePathInDir } from "core/util/ideUtils"; import { v4 as uuid } from "uuid"; -import { handleEditToolApplyError } from "../../redux/thunks/handleApplyStateUpdate"; +import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolImpl } from "./callClientTool"; export const singleFindAndReplaceImpl: ClientToolImpl = async ( @@ -62,23 +62,15 @@ export const singleFindAndReplaceImpl: ClientToolImpl = async ( } // Apply the changes to the file - void extras.ideMessenger - .request("applyToFile", { + void extras.dispatch( + applyForEditTool({ streamId, toolCallId, text: newContent, filepath: resolvedFilepath, isSearchAndReplace: true, - }) - .then((res) => { - if (res.status === "error") { - void extras.dispatch( - handleEditToolApplyError({ - toolCallId, - }), - ); - } - }); + }), + ); // Return success - applyToFile will handle the completion state return { From 3b6a290121d03141471b0ce391f8f679d2157edb Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Fri, 22 Aug 2025 15:42:14 -0700 Subject: [PATCH 07/14] fix: address bot feedback --- core/tools/definitions/multiEdit.ts | 2 +- .../FunctionSpecificToolCallDiv.tsx | 6 +- .../util/clientTools/findAndReplaceUtils.ts | 64 ++++++++ gui/src/util/clientTools/multiEditImpl.ts | 141 +++++++++--------- .../clientTools/singleFindAndReplaceImpl.ts | 41 ++--- 5 files changed, 148 insertions(+), 106 deletions(-) create mode 100644 gui/src/util/clientTools/findAndReplaceUtils.ts diff --git a/core/tools/definitions/multiEdit.ts b/core/tools/definitions/multiEdit.ts index edacb19120c..3003120000f 100644 --- a/core/tools/definitions/multiEdit.ts +++ b/core/tools/definitions/multiEdit.ts @@ -75,7 +75,7 @@ parameter is useful if you want to rename a variable for instance. If you want to create a new file, use: - A new file path - First edit: empty old_string and the new file's contents as new_string -- Subsequent edits: normal edit operations on the created content`, +- Subsequent edits are not allowed - there is no need since you are creating`, parameters: { type: "object", required: ["filepath", "edits"], diff --git a/gui/src/pages/gui/ToolCallDiv/FunctionSpecificToolCallDiv.tsx b/gui/src/pages/gui/ToolCallDiv/FunctionSpecificToolCallDiv.tsx index b86418183bd..ac74802a25d 100644 --- a/gui/src/pages/gui/ToolCallDiv/FunctionSpecificToolCallDiv.tsx +++ b/gui/src/pages/gui/ToolCallDiv/FunctionSpecificToolCallDiv.tsx @@ -33,9 +33,9 @@ function FunctionSpecificToolCallDiv({ /> ); case BuiltInToolNames.SearchAndReplaceInFile: - const changes = args.diffs + const changes = args?.diffs ? Array.isArray(args.diffs) - ? args.diffs.join("\n\n---\n\n") + ? args.diffs?.join("\n\n---\n\n") : args.diffs : ""; @@ -76,7 +76,7 @@ ${edit?.old_string ?? ""} ${edit?.new_string ?? ""} ===== REPLACE`, ) - .join("\n\n---\n\n") ?? ""; + ?.join("\n\n---\n\n") ?? ""; return ( 1) { + throw new Error( + `${errorContext}String "${oldString}" appears ${count} times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.`, + ); + } + + // Replace only the first occurrence + const firstIndex = content.indexOf(oldString); + return ( + content.substring(0, firstIndex) + + newString + + content.substring(firstIndex + oldString.length) + ); + } +} + +/** + * Validates a single edit operation + */ +export function validateSingleEdit( + oldString: string, + newString: string, + index?: number, +): void { + const context = index !== undefined ? `Edit #${index + 1}: ` : ""; + + if (!oldString && oldString !== "") { + throw new Error(`${context}old_string is required`); + } + if (newString === undefined) { + throw new Error(`${context}new_string is required`); + } + if (oldString === newString) { + throw new Error(`${context}old_string and new_string must be different`); + } +} diff --git a/gui/src/util/clientTools/multiEditImpl.ts b/gui/src/util/clientTools/multiEditImpl.ts index abe306a419f..b9e413bc23d 100644 --- a/gui/src/util/clientTools/multiEditImpl.ts +++ b/gui/src/util/clientTools/multiEditImpl.ts @@ -1,7 +1,14 @@ -import { resolveRelativePathInDir } from "core/util/ideUtils"; +import { + inferResolvedUriFromRelativePath, + resolveRelativePathInDir, +} from "core/util/ideUtils"; import { v4 as uuid } from "uuid"; import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolImpl } from "./callClientTool"; +import { + performFindAndReplace, + validateSingleEdit, +} from "./findAndReplaceUtils"; interface EditOperation { old_string: string; @@ -9,58 +16,26 @@ interface EditOperation { replace_all?: boolean; } -function validateEdits(edits: EditOperation[]): void { - for (let i = 0; i < edits.length; i++) { - const edit = edits[i]; - if (!edit.old_string && edit.old_string !== "") { - throw new Error(`Edit ${i + 1}: old_string is required`); - } - if (edit.new_string === undefined) { - throw new Error(`Edit ${i + 1}: new_string is required`); - } - if (edit.old_string === edit.new_string) { +export function validateCreating(edits: EditOperation[]) { + const isCreating = edits[0].old_string === ""; + if (edits.length > 1) { + if (isCreating) { throw new Error( - `Edit ${i + 1}: old_string and new_string must be different`, + "cannot make subsequent edits on a file you are creating", ); + } else { + for (let i = 1; i < edits.length; i++) { + if (edits[i].old_string === "") { + throw new Error( + `edit #${i + 1}: only the first edit can contain an empty old_string, which is only used for file creation.`, + ); + } + } } } -} - -function applyEdit( - content: string, - edit: EditOperation, - editIndex: number, - isFirstEditOfNewFile: boolean, -): string { - const { old_string, new_string, replace_all = false } = edit; - // For new file creation, the first edit can have empty old_string - if (isFirstEditOfNewFile && old_string === "") { - return new_string; - } - - // Check if old_string exists in current content - if (!content.includes(old_string)) { - throw new Error( - `Edit ${editIndex + 1}: String not found in file: "${old_string}"`, - ); - } - - if (replace_all) { - // Replace all occurrences - return content.split(old_string).join(new_string); - } else { - // Replace only the first occurrence, but check for uniqueness - const occurrences = content.split(old_string).length - 1; - if (occurrences > 1) { - throw new Error( - `Edit ${editIndex + 1}: String "${old_string}" appears ${occurrences} times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.`, - ); - } - return content.replace(old_string, new_string); - } + return isCreating; } - export const multiEditImpl: ClientToolImpl = async ( args, toolCallId, @@ -81,49 +56,69 @@ export const multiEditImpl: ClientToolImpl = async ( } // Validate each edit operation - validateEdits(edits); + for (let i = 0; i < edits.length; i++) { + const edit = edits[i]; + validateSingleEdit(edit.old_string, edit.new_string, i); + } // Check if this is creating a new file (first edit has empty old_string) - const isCreatingNewFile = edits[0].old_string === ""; - - // Resolve the file path - const resolvedFilepath = await resolveRelativePathInDir( + const isCreatingNewFile = validateCreating(edits); + const resolvedUri = await resolveRelativePathInDir( filepath, extras.ideMessenger.ide, ); - // For new files, resolvedFilepath might be null, so we construct the path - const targetFilepath = resolvedFilepath || filepath; - - if (!isCreatingNewFile) { - // For existing files, check if file exists - if (!resolvedFilepath) { - throw new Error(`File ${filepath} does not exist`); + let newContent: string; + let fileUri: string; + if (isCreatingNewFile) { + if (resolvedUri) { + throw new Error( + `file ${filepath} already exists, cannot create new file`, + ); } - } - - try { - // Read current file content (or start with empty for new files) - let currentContent = ""; - if (!isCreatingNewFile) { - currentContent = await extras.ideMessenger.ide.readFile(targetFilepath); + newContent = edits[0].new_string; + const response = await extras.ideMessenger.request( + "getWorkspaceDirs", + undefined, + ); + if (response.status === "error") { + throw new Error( + "Error getting workspace directories to infer file creation path", + ); } - - let newContent = currentContent; - - // Apply all edits sequentially + fileUri = await inferResolvedUriFromRelativePath( + filepath, + extras.ideMessenger.ide, + response.content, + ); + } else { + if (!resolvedUri) { + throw new Error( + `file ${filepath} does not exist. If you are trying to edit it, correct the filepath. If you are trying to create it, you must pass old_string=""`, + ); + } + newContent = await extras.ideMessenger.ide.readFile(resolvedUri); + fileUri = resolvedUri; for (let i = 0; i < edits.length; i++) { - const isFirstEditOfNewFile = i === 0 && isCreatingNewFile; - newContent = applyEdit(newContent, edits[i], i, isFirstEditOfNewFile); + const { old_string, new_string, replace_all } = edits[i]; + newContent = performFindAndReplace( + old_string, + edits[0], + new_string, + replace_all, + i, + ); } + } + try { // Apply the changes to the file void extras.dispatch( applyForEditTool({ streamId, toolCallId, text: newContent, - filepath: targetFilepath, + filepath: fileUri, isSearchAndReplace: true, }), ); diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts index b9ba5a9bb60..090d202e269 100644 --- a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts @@ -2,6 +2,10 @@ import { resolveRelativePathInDir } from "core/util/ideUtils"; import { v4 as uuid } from "uuid"; import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolImpl } from "./callClientTool"; +import { + performFindAndReplace, + validateSingleEdit, +} from "./findAndReplaceUtils"; export const singleFindAndReplaceImpl: ClientToolImpl = async ( args, @@ -16,15 +20,7 @@ export const singleFindAndReplaceImpl: ClientToolImpl = async ( if (!filepath) { throw new Error("filepath is required"); } - if (!old_string) { - throw new Error("old_string is required"); - } - if (new_string === undefined) { - throw new Error("new_string is required"); - } - if (old_string === new_string) { - throw new Error("old_string and new_string must be different"); - } + validateSingleEdit(old_string, new_string); // Resolve the file path const resolvedFilepath = await resolveRelativePathInDir( @@ -40,26 +36,13 @@ export const singleFindAndReplaceImpl: ClientToolImpl = async ( const originalContent = await extras.ideMessenger.ide.readFile(resolvedFilepath); - // Check if old_string exists in the file - if (!originalContent.includes(old_string)) { - throw new Error(`String not found in file: ${old_string}`); - } - - let newContent: string; - - if (replace_all) { - // Replace all occurrences - newContent = originalContent.split(old_string).join(new_string); - } else { - // Replace only the first occurrence - const occurrences = originalContent.split(old_string).length - 1; - if (occurrences > 1) { - throw new Error( - `String "${old_string}" appears ${occurrences} times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.`, - ); - } - newContent = originalContent.replace(old_string, new_string); - } + // Perform the find and replace operation + const newContent = performFindAndReplace( + originalContent, + old_string, + new_string, + replace_all, + ); // Apply the changes to the file void extras.dispatch( From 4f18953a451600b8f74c10ad4483d6c2043598ea Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Fri, 22 Aug 2025 15:59:00 -0700 Subject: [PATCH 08/14] fix: find and replace utils --- .../clientTools/findAndReplaceUtils.test.ts | 458 ++++++++++++++++++ .../util/clientTools/findAndReplaceUtils.ts | 34 +- gui/src/util/clientTools/multiEditImpl.ts | 31 +- 3 files changed, 492 insertions(+), 31 deletions(-) create mode 100644 gui/src/util/clientTools/findAndReplaceUtils.test.ts diff --git a/gui/src/util/clientTools/findAndReplaceUtils.test.ts b/gui/src/util/clientTools/findAndReplaceUtils.test.ts new file mode 100644 index 00000000000..53d541a8362 --- /dev/null +++ b/gui/src/util/clientTools/findAndReplaceUtils.test.ts @@ -0,0 +1,458 @@ +import { EditOperation } from "core/tools/definitions/multiEdit"; +import { describe, expect, it } from "vitest"; +import { + performFindAndReplace, + validateCreatingForMultiEdit, + validateSingleEdit, +} from "./findAndReplaceUtils"; + +describe("performFindAndReplace", () => { + describe("normal cases", () => { + it("should replace single occurrence by default", () => { + const content = "Hello world\nThis is a test file\nGoodbye"; + const result = performFindAndReplace(content, "Hello", "Hi"); + + expect(result).toBe("Hi world\nThis is a test file\nGoodbye"); + }); + + it("should replace single occurrence when replaceAll is false", () => { + const content = "Hello world\nHello again"; + const result = performFindAndReplace( + content, + "Hello world", + "Hi world", + false, + ); + + expect(result).toBe("Hi world\nHello again"); + }); + + it("should replace all occurrences when replaceAll is true", () => { + const content = "Hello world\nHello again\nHello there"; + const result = performFindAndReplace(content, "Hello", "Hi", true); + + expect(result).toBe("Hi world\nHi again\nHi there"); + }); + + it("should handle empty new string (deletion)", () => { + const content = "Hello world"; + const result = performFindAndReplace(content, "Hello ", ""); + + expect(result).toBe("world"); + }); + + it("should handle empty old string replacement (insertion at start)", () => { + const content = ""; + const result = performFindAndReplace(content, "", "Hello world"); + + expect(result).toBe("Hello world"); + }); + + it("should preserve whitespace and indentation", () => { + const content = + "function test() {\n const value = 'old';\n return value;\n}"; + const result = performFindAndReplace( + content, + " const value = 'old';", + " const value = 'new';", + ); + + expect(result).toBe( + "function test() {\n const value = 'new';\n return value;\n}", + ); + }); + + it("should handle multiline strings", () => { + const content = "line1\nline2\nline3"; + const result = performFindAndReplace( + content, + "line1\nline2", + "newline1\nnewline2", + ); + + expect(result).toBe("newline1\nnewline2\nline3"); + }); + }); + + describe("special character handling", () => { + it("should handle regex special characters in old string", () => { + const content = "const regex = /[a-z]+/g;"; + const result = performFindAndReplace(content, "/[a-z]+/g", "/[A-Z]+/g"); + + expect(result).toBe("const regex = /[A-Z]+/g;"); + }); + + it("should handle dollar signs in strings", () => { + const content = 'const text = "Hello $world"'; + const result = performFindAndReplace( + content, + '"Hello $world"', + '"Hi $universe"', + ); + + expect(result).toBe('const text = "Hi $universe"'); + }); + + it("should handle backslashes", () => { + const content = 'const path = "C:\\Users\\test"'; + const result = performFindAndReplace( + content, + "C:\\Users\\test", + "D:\\Users\\test", + ); + + expect(result).toBe('const path = "D:\\Users\\test"'); + }); + + it("should handle parentheses", () => { + const content = "function test() { return 'old'; }"; + const result = performFindAndReplace( + content, + "() { return 'old'; }", + "() { return 'new'; }", + ); + + expect(result).toBe("function test() { return 'new'; }"); + }); + + it("should handle square brackets", () => { + const content = "const arr = [1, 2, 3];"; + const result = performFindAndReplace(content, "[1, 2, 3]", "[4, 5, 6]"); + + expect(result).toBe("const arr = [4, 5, 6];"); + }); + }); + + describe("replaceAll behavior", () => { + it("should replace all occurrences including partial matches", () => { + const content = "test testing tested test"; + const result = performFindAndReplace(content, "test", "exam", true); + + expect(result).toBe("exam examing examed exam"); + }); + + it("should replace all occurrences with empty string", () => { + const content = "remove this remove that remove everything"; + const result = performFindAndReplace(content, "remove ", "", true); + + expect(result).toBe("this that everything"); + }); + + it("should handle overlapping patterns correctly", () => { + const content = "aaaa"; + const result = performFindAndReplace(content, "aa", "b", true); + + expect(result).toBe("bb"); + }); + }); + + describe("error cases", () => { + it("should throw error when string not found", () => { + const content = "Hello world"; + + expect(() => { + performFindAndReplace(content, "xyz", "abc"); + }).toThrow('String not found in file: "xyz"'); + }); + + it("should throw error with edit context when string not found", () => { + const content = "Hello world"; + + expect(() => { + performFindAndReplace(content, "xyz", "abc", false, 2); + }).toThrow('Edit #3: String not found in file: "xyz"'); + }); + + it("should throw error when multiple occurrences exist and replaceAll is false", () => { + const content = "Hello world\nHello again\nHello there"; + + expect(() => { + performFindAndReplace(content, "Hello", "Hi", false); + }).toThrow( + 'String "Hello" appears 3 times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.', + ); + }); + + it("should throw error with edit context for multiple occurrences", () => { + const content = "test test test"; + + expect(() => { + performFindAndReplace(content, "test", "exam", false, 1); + }).toThrow( + 'Edit #2: String "test" appears 3 times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.', + ); + }); + }); + + describe("edge cases", () => { + it("should handle single character replacement", () => { + const content = "a b c"; + const result = performFindAndReplace(content, "b", "x"); + + expect(result).toBe("a x c"); + }); + + it("should handle very long strings", () => { + const longString = "a".repeat(10000); + const content = `start${longString}end`; + const result = performFindAndReplace(content, longString, "short"); + + expect(result).toBe("startshortend"); + }); + + it("should handle strings at beginning of content", () => { + const content = "start middle end"; + const result = performFindAndReplace(content, "start", "begin"); + + expect(result).toBe("begin middle end"); + }); + + it("should handle strings at end of content", () => { + const content = "start middle end"; + const result = performFindAndReplace(content, "end", "finish"); + + expect(result).toBe("start middle finish"); + }); + + it("should handle entire content replacement", () => { + const content = "replace everything"; + const result = performFindAndReplace( + content, + "replace everything", + "new content", + ); + + expect(result).toBe("new content"); + }); + }); +}); + +describe("validateSingleEdit", () => { + describe("valid inputs", () => { + it("should not throw for valid non-empty strings", () => { + expect(() => { + validateSingleEdit("old", "new"); + }).not.toThrow(); + }); + + it("should allow empty old_string for file creation", () => { + expect(() => { + validateSingleEdit("", "new content"); + }).not.toThrow(); + }); + + it("should allow empty new_string for deletion", () => { + expect(() => { + validateSingleEdit("delete me", ""); + }).not.toThrow(); + }); + + it("should allow multiline strings", () => { + expect(() => { + validateSingleEdit("line1\nline2", "newline1\nnewline2"); + }).not.toThrow(); + }); + + it("should allow special characters", () => { + expect(() => { + validateSingleEdit("/[a-z]+/g", "/[A-Z]+/g"); + }).not.toThrow(); + }); + }); + + describe("invalid inputs", () => { + it("should throw error when old_string is null", () => { + expect(() => { + validateSingleEdit(null as any, "new"); + }).toThrow("old_string is required"); + }); + + it("should throw error when old_string is undefined", () => { + expect(() => { + validateSingleEdit(undefined as any, "new"); + }).toThrow("old_string is required"); + }); + + it("should throw error when new_string is undefined", () => { + expect(() => { + validateSingleEdit("old", undefined as any); + }).toThrow("new_string is required"); + }); + + it("should throw error when old_string and new_string are identical", () => { + expect(() => { + validateSingleEdit("same", "same"); + }).toThrow("old_string and new_string must be different"); + }); + + it("should throw error when both are empty strings", () => { + expect(() => { + validateSingleEdit("", ""); + }).toThrow("old_string and new_string must be different"); + }); + }); + + describe("error messages with context", () => { + it("should include edit number in error messages when index provided", () => { + expect(() => { + validateSingleEdit(null as any, "new", 2); + }).toThrow("Edit #3: old_string is required"); + }); + + it("should include edit number for new_string errors", () => { + expect(() => { + validateSingleEdit("old", undefined as any, 0); + }).toThrow("Edit #1: new_string is required"); + }); + + it("should include edit number for identical strings error", () => { + expect(() => { + validateSingleEdit("same", "same", 4); + }).toThrow("Edit #5: old_string and new_string must be different"); + }); + + it("should not include context when index not provided", () => { + expect(() => { + validateSingleEdit("same", "same"); + }).toThrow("old_string and new_string must be different"); + }); + }); +}); + +describe("validateCreatingForMultiEdit", () => { + describe("single edit scenarios", () => { + it("should return true for creating with single edit", () => { + const edits: EditOperation[] = [ + { old_string: "", new_string: "new file content" }, + ]; + + const isCreating = validateCreatingForMultiEdit(edits); + expect(isCreating).toBe(true); + }); + + it("should return false for editing with single edit", () => { + const edits: EditOperation[] = [{ old_string: "old", new_string: "new" }]; + + const isCreating = validateCreatingForMultiEdit(edits); + expect(isCreating).toBe(false); + }); + }); + + describe("multiple edit scenarios - editing existing file", () => { + it("should return false for multiple edits on existing file", () => { + const edits: EditOperation[] = [ + { old_string: "old1", new_string: "new1" }, + { old_string: "old2", new_string: "new2" }, + { old_string: "old3", new_string: "new3" }, + ]; + + const isCreating = validateCreatingForMultiEdit(edits); + expect(isCreating).toBe(false); + }); + + it("should allow multiple valid edits on existing file", () => { + const edits: EditOperation[] = [ + { old_string: "import old", new_string: "import new" }, + { old_string: "function old()", new_string: "function new()" }, + { old_string: "return old;", new_string: "return new;" }, + ]; + + expect(() => validateCreatingForMultiEdit(edits)).not.toThrow(); + expect(validateCreatingForMultiEdit(edits)).toBe(false); + }); + }); + + describe("error cases", () => { + it("should throw error when trying to make subsequent edits on file creation", () => { + const edits: EditOperation[] = [ + { old_string: "", new_string: "new file content" }, + { old_string: "old", new_string: "new" }, + ]; + + expect(() => { + validateCreatingForMultiEdit(edits); + }).toThrow("cannot make subsequent edits on a file you are creating"); + }); + + it("should throw error when empty old_string appears in non-first edit", () => { + const edits: EditOperation[] = [ + { old_string: "old1", new_string: "new1" }, + { old_string: "", new_string: "new2" }, + ]; + + expect(() => { + validateCreatingForMultiEdit(edits); + }).toThrow( + "edit #2: only the first edit can contain an empty old_string, which is only used for file creation.", + ); + }); + + it("should throw error with correct edit number for empty old_string", () => { + const edits: EditOperation[] = [ + { old_string: "old1", new_string: "new1" }, + { old_string: "old2", new_string: "new2" }, + { old_string: "", new_string: "new3" }, + ]; + + expect(() => { + validateCreatingForMultiEdit(edits); + }).toThrow( + "edit #3: only the first edit can contain an empty old_string, which is only used for file creation.", + ); + }); + + it("should throw error for multiple empty old_strings in subsequent edits", () => { + const edits: EditOperation[] = [ + { old_string: "old1", new_string: "new1" }, + { old_string: "", new_string: "new2" }, + { old_string: "", new_string: "new3" }, + ]; + + expect(() => { + validateCreatingForMultiEdit(edits); + }).toThrow( + "edit #2: only the first edit can contain an empty old_string, which is only used for file creation.", + ); + }); + }); + + describe("edge cases", () => { + it("should handle empty edits array gracefully", () => { + const edits: EditOperation[] = []; + + // This should not throw, but would likely cause issues elsewhere + // The function doesn't explicitly handle empty arrays + expect(() => { + validateCreatingForMultiEdit(edits); + }).toThrow(); // Will throw because edits[0] is undefined + }); + + it("should handle complex edit operations", () => { + const edits: EditOperation[] = [ + { + old_string: "function oldFunction() {\n return 'old';\n}", + new_string: "function newFunction() {\n return 'new';\n}", + replace_all: false, + }, + { + old_string: "const OLD_CONSTANT", + new_string: "const NEW_CONSTANT", + replace_all: true, + }, + ]; + + expect(() => validateCreatingForMultiEdit(edits)).not.toThrow(); + expect(validateCreatingForMultiEdit(edits)).toBe(false); + }); + + it("should handle edits with replace_all flags", () => { + const edits: EditOperation[] = [ + { old_string: "old", new_string: "new", replace_all: true }, + { old_string: "test", new_string: "exam", replace_all: false }, + ]; + + expect(() => validateCreatingForMultiEdit(edits)).not.toThrow(); + expect(validateCreatingForMultiEdit(edits)).toBe(false); + }); + }); +}); diff --git a/gui/src/util/clientTools/findAndReplaceUtils.ts b/gui/src/util/clientTools/findAndReplaceUtils.ts index c2554d00dda..b10ce6adc9d 100644 --- a/gui/src/util/clientTools/findAndReplaceUtils.ts +++ b/gui/src/util/clientTools/findAndReplaceUtils.ts @@ -1,3 +1,5 @@ +import { EditOperation } from "core/tools/definitions/multiEdit"; + /** * Performs a find and replace operation on text content with proper handling of special characters */ @@ -18,12 +20,17 @@ export function performFindAndReplace( // Replace all occurrences using replaceAll for proper handling of special characters return content.replaceAll(oldString, newString); } else { + // Handle empty oldString case (insertion) + if (oldString === "") { + return newString + content; + } + // Count occurrences using indexOf for proper handling of special characters let count = 0; - let index = content.indexOf(oldString); - while (index !== -1) { + let searchIndex = content.indexOf(oldString); + while (searchIndex !== -1) { count++; - index = content.indexOf(oldString, index + 1); + searchIndex = content.indexOf(oldString, searchIndex + 1); } if (count > 1) { @@ -62,3 +69,24 @@ export function validateSingleEdit( throw new Error(`${context}old_string and new_string must be different`); } } + +export function validateCreatingForMultiEdit(edits: EditOperation[]) { + const isCreating = edits[0].old_string === ""; + if (edits.length > 1) { + if (isCreating) { + throw new Error( + "cannot make subsequent edits on a file you are creating", + ); + } else { + for (let i = 1; i < edits.length; i++) { + if (edits[i].old_string === "") { + throw new Error( + `edit #${i + 1}: only the first edit can contain an empty old_string, which is only used for file creation.`, + ); + } + } + } + } + + return isCreating; +} diff --git a/gui/src/util/clientTools/multiEditImpl.ts b/gui/src/util/clientTools/multiEditImpl.ts index b9e413bc23d..7e14b21fafa 100644 --- a/gui/src/util/clientTools/multiEditImpl.ts +++ b/gui/src/util/clientTools/multiEditImpl.ts @@ -7,35 +7,10 @@ import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolImpl } from "./callClientTool"; import { performFindAndReplace, + validateCreatingForMultiEdit, validateSingleEdit, } from "./findAndReplaceUtils"; -interface EditOperation { - old_string: string; - new_string: string; - replace_all?: boolean; -} - -export function validateCreating(edits: EditOperation[]) { - const isCreating = edits[0].old_string === ""; - if (edits.length > 1) { - if (isCreating) { - throw new Error( - "cannot make subsequent edits on a file you are creating", - ); - } else { - for (let i = 1; i < edits.length; i++) { - if (edits[i].old_string === "") { - throw new Error( - `edit #${i + 1}: only the first edit can contain an empty old_string, which is only used for file creation.`, - ); - } - } - } - } - - return isCreating; -} export const multiEditImpl: ClientToolImpl = async ( args, toolCallId, @@ -62,7 +37,7 @@ export const multiEditImpl: ClientToolImpl = async ( } // Check if this is creating a new file (first edit has empty old_string) - const isCreatingNewFile = validateCreating(edits); + const isCreatingNewFile = validateCreatingForMultiEdit(edits); const resolvedUri = await resolveRelativePathInDir( filepath, extras.ideMessenger.ide, @@ -102,8 +77,8 @@ export const multiEditImpl: ClientToolImpl = async ( for (let i = 0; i < edits.length; i++) { const { old_string, new_string, replace_all } = edits[i]; newContent = performFindAndReplace( + newContent, old_string, - edits[0], new_string, replace_all, i, From c3ad86f8765141ca1f8f333aa00759c3fbfdb77a Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Fri, 22 Aug 2025 16:14:38 -0700 Subject: [PATCH 09/14] fix: gui tests for edit tools --- .../util/clientTools/multiEditImpl.test.ts | 79 +++++++++---------- gui/src/util/clientTools/multiEditImpl.ts | 48 ++++------- .../singleFindAndReplaceImpl.test.ts | 4 +- .../clientTools/singleFindAndReplaceImpl.ts | 58 ++++++-------- 4 files changed, 83 insertions(+), 106 deletions(-) diff --git a/gui/src/util/clientTools/multiEditImpl.test.ts b/gui/src/util/clientTools/multiEditImpl.test.ts index f8069bff130..e98ff7ec1ed 100644 --- a/gui/src/util/clientTools/multiEditImpl.test.ts +++ b/gui/src/util/clientTools/multiEditImpl.test.ts @@ -10,6 +10,7 @@ vi.mock("uuid", () => ({ vi.mock("core/util/ideUtils", () => ({ resolveRelativePathInDir: vi.fn(), + inferResolvedUriFromRelativePath: vi.fn(), })); vi.mock("../../redux/thunks/handleApplyStateUpdate", () => ({ @@ -19,12 +20,16 @@ vi.mock("../../redux/thunks/handleApplyStateUpdate", () => ({ describe("multiEditImpl", () => { let mockExtras: ClientToolExtras; let mockResolveRelativePathInDir: Mock; + let mockInferResolvedUriFromRelativePath: Mock; let mockApplyForEditTool: Mock; beforeEach(() => { vi.clearAllMocks(); mockResolveRelativePathInDir = vi.mocked(ideUtils.resolveRelativePathInDir); + mockInferResolvedUriFromRelativePath = vi.mocked( + ideUtils.inferResolvedUriFromRelativePath, + ); mockApplyForEditTool = vi.mocked(applyForEditTool); mockExtras = { @@ -33,7 +38,10 @@ describe("multiEditImpl", () => { })) as any, dispatch: vi.fn() as any, ideMessenger: { - ide: { readFile: vi.fn() }, + ide: { + readFile: vi.fn(), + getWorkspaceDirs: vi.fn().mockResolvedValue(["dir1"]), + }, request: vi.fn(), } as any, }; @@ -64,7 +72,7 @@ describe("multiEditImpl", () => { "id", mockExtras, ), - ).rejects.toThrow("Edit 1: old_string is required"); + ).rejects.toThrow("Edit #1: old_string is required"); }); it("should throw if edit has undefined new_string", async () => { @@ -77,7 +85,7 @@ describe("multiEditImpl", () => { "id", mockExtras, ), - ).rejects.toThrow("Edit 1: new_string is required"); + ).rejects.toThrow("Edit #1: new_string is required"); }); it("should throw if old_string equals new_string", async () => { @@ -90,13 +98,15 @@ describe("multiEditImpl", () => { "id", mockExtras, ), - ).rejects.toThrow("Edit 1: old_string and new_string must be different"); + ).rejects.toThrow("Edit #1: old_string and new_string must be different"); }); }); describe("sequential edits", () => { beforeEach(() => { - mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + mockResolveRelativePathInDir.mockResolvedValue( + "file:///dir/test/file.txt", + ); }); it("should apply single edit", async () => { @@ -117,7 +127,7 @@ describe("multiEditImpl", () => { streamId: "test-uuid", toolCallId: "id", text: "Hi world", - filepath: "/test/file.txt", + filepath: "file:///dir/test/file.txt", isSearchAndReplace: true, }); }); @@ -143,7 +153,7 @@ describe("multiEditImpl", () => { streamId: "test-uuid", toolCallId: "id", text: "Hi universe\nGoodbye universe", - filepath: "/test/file.txt", + filepath: "file:///dir/test/file.txt", isSearchAndReplace: true, }); }); @@ -169,7 +179,7 @@ describe("multiEditImpl", () => { streamId: "test-uuid", toolCallId: "id", text: "let x = 2;", - filepath: "/test/file.txt", + filepath: "file:///dir/test/file.txt", isSearchAndReplace: true, }); }); @@ -191,7 +201,7 @@ describe("multiEditImpl", () => { "id", mockExtras, ), - ).rejects.toThrow('Edit 2: String not found in file: "not found"'); + ).rejects.toThrow('Edit #2: String not found in file: "not found"'); }); it("should throw if multiple occurrences without replace_all", async () => { @@ -208,13 +218,18 @@ describe("multiEditImpl", () => { "id", mockExtras, ), - ).rejects.toThrow('Edit 1: String "test" appears 3 times'); + ).rejects.toThrow( + 'Edit #1: String "test" appears 3 times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.', + ); }); }); describe("file creation", () => { it("should create new file with empty old_string", async () => { mockResolveRelativePathInDir.mockResolvedValue(null); + mockInferResolvedUriFromRelativePath.mockResolvedValue( + "file:///infered/new.txt", + ); await multiEditImpl( { @@ -229,31 +244,7 @@ describe("multiEditImpl", () => { streamId: "test-uuid", toolCallId: "id", text: "New content\nLine 2", - filepath: "new.txt", - isSearchAndReplace: true, - }); - }); - - it("should create and edit new file", async () => { - mockResolveRelativePathInDir.mockResolvedValue(null); - - await multiEditImpl( - { - filepath: "new.txt", - edits: [ - { old_string: "", new_string: "Hello world" }, - { old_string: "world", new_string: "universe" }, - ], - }, - "id", - mockExtras, - ); - - expect(mockApplyForEditTool).toHaveBeenCalledWith({ - streamId: "test-uuid", - toolCallId: "id", - text: "Hello universe", - filepath: "new.txt", + filepath: "file:///infered/new.txt", isSearchAndReplace: true, }); }); @@ -261,7 +252,9 @@ describe("multiEditImpl", () => { describe("replace_all functionality", () => { beforeEach(() => { - mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + mockResolveRelativePathInDir.mockResolvedValue( + "file:///dir/test/file.txt", + ); }); it("should replace all occurrences when specified", async () => { @@ -282,7 +275,7 @@ describe("multiEditImpl", () => { streamId: "test-uuid", toolCallId: "id", text: "qux bar qux baz qux", - filepath: "/test/file.txt", + filepath: "file:///dir/test/file.txt", isSearchAndReplace: true, }); }); @@ -308,7 +301,7 @@ describe("multiEditImpl", () => { streamId: "test-uuid", toolCallId: "id", text: "a b a z a", - filepath: "/test/file.txt", + filepath: "file:///dir/test/file.txt", isSearchAndReplace: true, }); }); @@ -316,7 +309,9 @@ describe("multiEditImpl", () => { describe("error handling", () => { it("should wrap readFile errors", async () => { - mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + mockResolveRelativePathInDir.mockResolvedValue( + "file:///dir/test/file.txt", + ); mockExtras.ideMessenger.ide.readFile = vi .fn() .mockRejectedValue(new Error("Read failed")); @@ -330,13 +325,15 @@ describe("multiEditImpl", () => { "id", mockExtras, ), - ).rejects.toThrow("Failed to apply multi edit: Read failed"); + ).rejects.toThrow("Read failed"); }); }); describe("return value", () => { it("should return correct structure", async () => { - mockResolveRelativePathInDir.mockResolvedValue("/test/file.txt"); + mockResolveRelativePathInDir.mockResolvedValue( + "file:///dir/test/file.txt", + ); mockExtras.ideMessenger.ide.readFile = vi.fn().mockResolvedValue("test"); const result = await multiEditImpl( diff --git a/gui/src/util/clientTools/multiEditImpl.ts b/gui/src/util/clientTools/multiEditImpl.ts index 7e14b21fafa..66d1167231d 100644 --- a/gui/src/util/clientTools/multiEditImpl.ts +++ b/gui/src/util/clientTools/multiEditImpl.ts @@ -52,19 +52,11 @@ export const multiEditImpl: ClientToolImpl = async ( ); } newContent = edits[0].new_string; - const response = await extras.ideMessenger.request( - "getWorkspaceDirs", - undefined, - ); - if (response.status === "error") { - throw new Error( - "Error getting workspace directories to infer file creation path", - ); - } + const dirs = await extras.ideMessenger.ide.getWorkspaceDirs(); fileUri = await inferResolvedUriFromRelativePath( filepath, extras.ideMessenger.ide, - response.content, + dirs, ); } else { if (!resolvedUri) { @@ -86,26 +78,20 @@ export const multiEditImpl: ClientToolImpl = async ( } } - try { - // Apply the changes to the file - void extras.dispatch( - applyForEditTool({ - streamId, - toolCallId, - text: newContent, - filepath: fileUri, - isSearchAndReplace: true, - }), - ); + // Apply the changes to the file + void extras.dispatch( + applyForEditTool({ + streamId, + toolCallId, + text: newContent, + filepath: fileUri, + isSearchAndReplace: true, + }), + ); - // Return success - applyToFile will handle the completion state - return { - respondImmediately: false, // Let apply state handle completion - output: undefined, - }; - } catch (error) { - throw new Error( - `Failed to apply multi edit: ${error instanceof Error ? error.message : String(error)}`, - ); - } + // Return success - applyToFile will handle the completion state + return { + respondImmediately: false, // Let apply state handle completion + output: undefined, + }; }; diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts index 0b380dc1880..3606a0b4050 100644 --- a/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts @@ -148,7 +148,7 @@ describe("singleFindAndReplaceImpl", () => { await expect( singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), - ).rejects.toThrow("String not found in file: not found"); + ).rejects.toThrow(`String not found in file: "not found"`); }); it("should replace single occurrence", async () => { @@ -328,7 +328,7 @@ describe("singleFindAndReplaceImpl", () => { await expect( singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), - ).rejects.toThrow("Failed to apply find and replace: Permission denied"); + ).rejects.toThrow("Permission denied"); }); }); }); diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts index 090d202e269..aec0ebf816a 100644 --- a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts @@ -31,38 +31,32 @@ export const singleFindAndReplaceImpl: ClientToolImpl = async ( throw new Error(`File ${filepath} does not exist`); } - try { - // Read the current file content - const originalContent = - await extras.ideMessenger.ide.readFile(resolvedFilepath); - - // Perform the find and replace operation - const newContent = performFindAndReplace( - originalContent, - old_string, - new_string, - replace_all, - ); + // Read the current file content + const originalContent = + await extras.ideMessenger.ide.readFile(resolvedFilepath); + + // Perform the find and replace operation + const newContent = performFindAndReplace( + originalContent, + old_string, + new_string, + replace_all, + ); - // Apply the changes to the file - void extras.dispatch( - applyForEditTool({ - streamId, - toolCallId, - text: newContent, - filepath: resolvedFilepath, - isSearchAndReplace: true, - }), - ); + // Apply the changes to the file + void extras.dispatch( + applyForEditTool({ + streamId, + toolCallId, + text: newContent, + filepath: resolvedFilepath, + isSearchAndReplace: true, + }), + ); - // Return success - applyToFile will handle the completion state - return { - respondImmediately: false, // Let apply state handle completion - output: undefined, - }; - } catch (error) { - throw new Error( - `Failed to apply find and replace: ${error instanceof Error ? error.message : String(error)}`, - ); - } + // Return success - applyToFile will handle the completion state + return { + respondImmediately: false, // Let apply state handle completion + output: undefined, + }; }; From 469c0c8e73965e2c98d277e846e56a12b1e47e65 Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Sat, 23 Aug 2025 16:37:18 -0700 Subject: [PATCH 10/14] feat: UI for diff tools --- gui/package-lock.json | 20 +- gui/package.json | 2 + .../StepContainerPreToolbar/ApplyActions.tsx | 2 +- .../StepContainerPreToolbar/FileInfo.tsx | 3 +- .../StepContainerPreToolbar/index.tsx | 2 +- .../gui/ToolCallDiv/FindAndReplace.test.tsx | 209 ++++++++++++++ .../pages/gui/ToolCallDiv/FindAndReplace.tsx | 269 ++++++++++++++++++ .../FunctionSpecificToolCallDiv.tsx | 42 ++- gui/src/redux/slices/sessionSlice.ts | 16 ++ gui/src/redux/thunks/enhanceParsedArgs.ts | 53 ++++ gui/src/redux/thunks/streamNormalInput.ts | 15 + gui/src/util/clientTools/multiEditImpl.ts | 6 +- gui/src/util/clientTools/searchReplaceImpl.ts | 5 +- .../clientTools/singleFindAndReplaceImpl.ts | 11 +- 14 files changed, 620 insertions(+), 35 deletions(-) create mode 100644 gui/src/pages/gui/ToolCallDiv/FindAndReplace.test.tsx create mode 100644 gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx create mode 100644 gui/src/redux/thunks/enhanceParsedArgs.ts diff --git a/gui/package-lock.json b/gui/package-lock.json index a3dffda4cd5..8e4bfb7d9d6 100644 --- a/gui/package-lock.json +++ b/gui/package-lock.json @@ -27,10 +27,12 @@ "@tiptap/react": "^2.1.13", "@tiptap/starter-kit": "^2.1.13", "@tiptap/suggestion": "^2.1.13", + "@types/diff": "^7.0.2", "@types/uuid": "^10.0.0", "anser": "^2.3.2", "clsx": "^2.1.1", "core": "file:../core", + "diff": "^8.0.2", "dompurify": "^3.0.6", "downshift": "^7.6.0", "escape-carriage": "^1.3.1", @@ -129,7 +131,6 @@ "@sentry/esbuild-plugin": "^4.0.2", "@sentry/node": "^9.43.0", "@sentry/vite-plugin": "^4.0.2", - "@typescript-eslint/eslint-plugin": "^7.8.0", "@xenova/transformers": "2.14.0", "adf-to-md": "^1.1.0", "async-mutex": "^0.5.0", @@ -209,6 +210,8 @@ "@types/tar": "^6.1.13", "@types/uuid": "^9.0.7", "@types/win-ca": "^3.5.4", + "@typescript-eslint/eslint-plugin": "^8.40.0", + "@typescript-eslint/parser": "^8.40.0", "cross-env": "^7.0.3", "esbuild": "0.17.19", "eslint": "^8", @@ -3865,6 +3868,12 @@ "@types/ms": "*" } }, + "node_modules/@types/diff": { + "version": "7.0.2", + "resolved": "https://registry.npmjs.org/@types/diff/-/diff-7.0.2.tgz", + "integrity": "sha512-JSWRMozjFKsGlEjiiKajUjIJVKuKdE3oVy2DNtK+fUo8q82nhFZ2CPQwicAIkXrofahDXrWJ7mjelvZphMS98Q==", + "license": "MIT" + }, "node_modules/@types/dompurify": { "version": "3.2.0", "resolved": "https://registry.npmjs.org/@types/dompurify/-/dompurify-3.2.0.tgz", @@ -6194,6 +6203,15 @@ "integrity": "sha512-gxtyfqMg7GKyhQmb056K7M3xszy/myH8w+B4RT+QXBQsvAOdc3XymqDDPHx1BgPgsdAA5SIifona89YtRATDzw==", "dev": true }, + "node_modules/diff": { + "version": "8.0.2", + "resolved": "https://registry.npmjs.org/diff/-/diff-8.0.2.tgz", + "integrity": "sha512-sSuxWU5j5SR9QQji/o2qMvqRNYRDOcBTgsJ/DeCf4iSN4gW+gNMXM7wFIP+fdXZxoNiAnHUTGjCr+TSWXdRDKg==", + "license": "BSD-3-Clause", + "engines": { + "node": ">=0.3.1" + } + }, "node_modules/direction": { "version": "1.0.4", "resolved": "https://registry.npmjs.org/direction/-/direction-1.0.4.tgz", diff --git a/gui/package.json b/gui/package.json index ab83efb22fd..6cb45030c9c 100644 --- a/gui/package.json +++ b/gui/package.json @@ -36,10 +36,12 @@ "@tiptap/react": "^2.1.13", "@tiptap/starter-kit": "^2.1.13", "@tiptap/suggestion": "^2.1.13", + "@types/diff": "^7.0.2", "@types/uuid": "^10.0.0", "anser": "^2.3.2", "clsx": "^2.1.1", "core": "file:../core", + "diff": "^8.0.2", "dompurify": "^3.0.6", "downshift": "^7.6.0", "escape-carriage": "^1.3.1", diff --git a/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/ApplyActions.tsx b/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/ApplyActions.tsx index 1214b9e2425..9b7ba953d2a 100644 --- a/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/ApplyActions.tsx +++ b/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/ApplyActions.tsx @@ -11,7 +11,7 @@ interface ApplyActionsProps { applyState?: ApplyState; onClickAccept: () => void; onClickReject: () => void; - onClickApply: () => void; + onClickApply?: () => void; } export function ApplyActions(props: ApplyActionsProps) { diff --git a/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/FileInfo.tsx b/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/FileInfo.tsx index 550d1fd284e..c288c7cd095 100644 --- a/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/FileInfo.tsx +++ b/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/FileInfo.tsx @@ -1,10 +1,11 @@ import { getLastNPathParts } from "core/util/uri"; +import { MouseEventHandler } from "react"; import FileIcon from "../../FileIcon"; import { ToolTip } from "../../gui/Tooltip"; export interface FileInfoProps { filepath: string; - onClick?: () => void; + onClick?: MouseEventHandler; range?: string; } diff --git a/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/index.tsx b/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/index.tsx index e97c13061f6..f9d268ccc3f 100644 --- a/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/index.tsx +++ b/gui/src/components/StyledMarkdownPreview/StepContainerPreToolbar/index.tsx @@ -96,7 +96,7 @@ export function StepContainerPreToolbar({ toolCallState?.status === "errored" || toolCallState?.status === "done") ? (
{ if (toolCallState.output) { ideMessenger.post("showVirtualFile", { diff --git a/gui/src/pages/gui/ToolCallDiv/FindAndReplace.test.tsx b/gui/src/pages/gui/ToolCallDiv/FindAndReplace.test.tsx new file mode 100644 index 00000000000..3e912bd66a8 --- /dev/null +++ b/gui/src/pages/gui/ToolCallDiv/FindAndReplace.test.tsx @@ -0,0 +1,209 @@ +import { configureStore } from "@reduxjs/toolkit"; +import { render, screen } from "@testing-library/react"; +import { ApplyState } from "core"; +import { Provider } from "react-redux"; +import { describe, expect, it, Mock, vi } from "vitest"; +import { useFileContent } from "../../../hooks/useFileContent"; +import { SingleFindAndReplace } from "./FindAndReplace"; + +// Mock the useFileContent hook +vi.mock("../../../hooks/useFileContent"); + +// Mock the selectApplyStateByToolCallId selector +vi.mock("../../../redux/selectors/selectToolCalls", () => ({ + selectApplyStateByToolCallId: vi.fn(), +})); + +const mockUseFileContent = useFileContent as Mock; + +// Create a minimal store for testing +const createTestStore = (applyState?: ApplyState) => { + return configureStore({ + reducer: { + session: () => ({ + codeBlockApplyStates: { + states: applyState ? [applyState] : [], + }, + }), + }, + }); +}; + +describe("SingleFindAndReplace", () => { + const defaultProps = { + relativeFilePath: "test.ts", + oldString: "old code", + newString: "new code", + replaceAll: false, + toolCallId: "test-tool-call-id", + historyIndex: 0, + }; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("shows loading state when file content is loading", () => { + mockUseFileContent.mockReturnValue({ + fileContent: null, + isLoading: true, + error: null, + }); + + const store = createTestStore(); + render( + + + , + ); + + expect(screen.getByText("Loading file content...")).toBeInTheDocument(); + }); + + it("shows error when file content fails to load", () => { + mockUseFileContent.mockReturnValue({ + fileContent: null, + isLoading: false, + error: "File not found", + }); + + const store = createTestStore(); + render( + + + , + ); + + expect( + screen.getByText(`Failed to load file: ${defaultProps.relativeFilePath}`), + ).toBeInTheDocument(); + }); + + it("shows error when find and replace operation fails", () => { + mockUseFileContent.mockReturnValue({ + fileContent: "some content without the old string", + isLoading: false, + error: null, + }); + + const store = createTestStore(); + render( + + + , + ); + + expect(screen.getByText(/Error:/)).toBeInTheDocument(); + expect(screen.getByText(/String not found in file/)).toBeInTheDocument(); + }); + + it("shows no changes message when old and new strings result in same content", () => { + mockUseFileContent.mockReturnValue({ + fileContent: "some content", + isLoading: false, + error: null, + }); + + const store = createTestStore(); + render( + + + , + ); + + // This should show an error since the string doesn't exist + expect(screen.getByText(/Error:/)).toBeInTheDocument(); + }); + + it("shows diff when find and replace operation succeeds", () => { + const fileContent = "Hello old code world"; + mockUseFileContent.mockReturnValue({ + fileContent, + isLoading: false, + error: null, + }); + + const store = createTestStore(); + render( + + + , + ); + + expect(screen.getByText(defaultProps.relativeFilePath)).toBeInTheDocument(); + // The diff should show the old line being removed and new line being added + expect(screen.getByText("Hello old code world")).toBeInTheDocument(); + expect(screen.getByText("Hello new code world")).toBeInTheDocument(); + }); + + it("shows apply state status when available", () => { + mockUseFileContent.mockReturnValue({ + fileContent: "Hello old code world", + isLoading: false, + error: null, + }); + + const applyState: ApplyState = { + streamId: "test-stream", + status: "streaming", + toolCallId: "test-tool-call-id", + }; + + const store = createTestStore(applyState); + render( + + + , + ); + + expect(screen.getByText("Applying...")).toBeInTheDocument(); + }); + + it("shows tool call status icon when enabled", () => { + mockUseFileContent.mockReturnValue({ + fileContent: "Hello old code world", + isLoading: false, + error: null, + }); + + const store = createTestStore(); + render( + + + , + ); + + // Look for the status indicator (green dot for done status) + const statusIndicator = document.querySelector(".bg-green-500"); + expect(statusIndicator).toBeInTheDocument(); + }); + + it("handles replace all option correctly", () => { + const fileContent = "old code and more old code"; + mockUseFileContent.mockReturnValue({ + fileContent, + isLoading: false, + error: null, + }); + + const store = createTestStore(); + render( + + + , + ); + + // Should show both replacements in the diff + expect(screen.getByText(defaultProps.relativeFilePath)).toBeInTheDocument(); + expect(screen.getByText("old code and more old code")).toBeInTheDocument(); + expect(screen.getByText("new code and more new code")).toBeInTheDocument(); + }); +}); diff --git a/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx b/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx new file mode 100644 index 00000000000..55cf27ae31e --- /dev/null +++ b/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx @@ -0,0 +1,269 @@ +import { ChevronDownIcon } from "@heroicons/react/24/outline"; +import { ApplyState } from "core"; +import { EditOperation } from "core/tools/definitions/multiEdit"; +import { renderContextItems } from "core/util/messageContent"; +import { getLastNPathParts, getUriPathBasename } from "core/util/uri"; +import { diffLines } from "diff"; +import { useContext, useMemo, useState } from "react"; +import { ApplyActions } from "../../../components/StyledMarkdownPreview/StepContainerPreToolbar/ApplyActions"; +import { FileInfo } from "../../../components/StyledMarkdownPreview/StepContainerPreToolbar/FileInfo"; +import { useFontSize } from "../../../components/ui"; +import { IdeMessengerContext } from "../../../context/IdeMessenger"; +import { useAppSelector } from "../../../redux/hooks"; +import { + selectApplyStateByToolCallId, + selectToolCallById, +} from "../../../redux/selectors/selectToolCalls"; +import { performFindAndReplace } from "../../../util/clientTools/findAndReplaceUtils"; +import { getStatusIcon } from "./utils"; + +interface FindAndReplaceDisplayProps { + fileUri?: string; + editingFileContents?: string; + relativeFilePath?: string; + edits: EditOperation[]; + toolCallId: string; + historyIndex: number; +} + +export function FindAndReplaceDisplay({ + fileUri, + relativeFilePath, + editingFileContents, + edits, + toolCallId, + historyIndex, +}: FindAndReplaceDisplayProps) { + const [isExpanded, setIsExpanded] = useState(false); + const ideMessenger = useContext(IdeMessengerContext); + const applyState: ApplyState | undefined = useAppSelector((state) => + selectApplyStateByToolCallId(state, toolCallId), + ); + const fontSize = useFontSize(); + const toolCallState = useAppSelector((state) => + selectToolCallById(state, toolCallId), + ); + const config = useAppSelector((state) => state.config.config); + + const displayName = useMemo(() => { + if (fileUri) { + return getUriPathBasename(fileUri); + } + if (relativeFilePath) { + return getLastNPathParts(relativeFilePath, 1); + } + return ""; + }, [fileUri, relativeFilePath]); + + // Get file content from tool call state instead of reading file + const currentFileContent = useMemo(() => { + if (editingFileContents) { + return editingFileContents; + } + return edits.map((edit) => edit.old_string).join("\n"); + }, [editingFileContents, edits]); + + const diffResult = useMemo(() => { + if (!currentFileContent) { + return null; + } + + try { + // Apply all edits sequentially + let newContent = currentFileContent; + for (let i = 0; i < edits.length; i++) { + const { + old_string: oldString, + new_string: newString, + replace_all: replaceAll, + } = edits[i]; + newContent = performFindAndReplace( + newContent, + oldString, + newString, + replaceAll, + i, + ); + } + + // Generate diff between original and final content + const diff = diffLines(currentFileContent, newContent); + return { diff, newContent, error: null }; + } catch (error) { + return { + diff: null, + newContent: null, + error: error instanceof Error ? error.message : "Unknown error", + }; + } + }, [currentFileContent, edits]); + + const statusIcon = useMemo(() => { + const status = toolCallState?.status; + if (status) { + if (status === "canceled" || status === "errored" || status === "done") { + return ( +
{ + if (toolCallState.output) { + e.stopPropagation(); + ideMessenger.post("showVirtualFile", { + name: "Edit output", + content: renderContextItems(toolCallState.output), + }); + } + }} + > + {getStatusIcon(toolCallState.status)} +
+ ); + } + } + }, [toolCallState?.status]); + + // Unified container component that always renders the same structure + const renderContainer = (content: React.ReactNode) => ( +
+
{ + setIsExpanded((prev) => !prev); + }} + > +
+ {statusIcon} + + { + if (!fileUri) { + return; + } + e.stopPropagation(); + ideMessenger.post("openFile", { path: fileUri }); + }} + /> +
+ + {applyState && ( + { + ideMessenger.post(`acceptDiff`, { + filepath: fileUri, + streamId: applyState.streamId, + }); + }} + onClickReject={() => { + ideMessenger.post(`rejectDiff`, { + filepath: fileUri, + streamId: applyState.streamId, + }); + }} + disableManualApply={true} + applyState={applyState} + /> + )} +
+ {toolCallState?.status === "generated" || isExpanded ? content : null} +
+ ); + + if (diffResult?.error) { + return renderContainer( +
+ Error generating diff {diffResult.error} +
, + ); + } + + if (!diffResult?.diff) { + return renderContainer( +
No changes to display
, + ); + } + + return renderContainer( +
+ 
+        {diffResult.diff.map((part, index) => {
+          if (part.removed) {
+            return (
+              

+                {part.value.split("\n").map((line, lineIndex) => {
+                  if (
+                    line === "" &&
+                    lineIndex === part.value.split("\n").length - 1
+                  )
+                    return null;
+                  return (
+                    

+                      -
+                      {line}
+                    

+                  );
+                })}
+              

+            );
+          } else if (part.added) {
+            return (
+              

+                {part.value.split("\n").map((line, lineIndex) => {
+                  if (
+                    line === "" &&
+                    lineIndex === part.value.split("\n").length - 1
+                  )
+                    return null;
+                  return (
+                    

+                      +
+                      {line}
+                    

+                  );
+                })}
+              

+            );
+          } else {
+            return (
+              

+                {part.value.split("\n").map((line, lineIndex) => {
+                  if (
+                    line === "" &&
+                    lineIndex === part.value.split("\n").length - 1
+                  )
+                    return null;
+                  return (
+                    

+                      
+                        {" "}
+                      
+                      {line}
+                    

+                  );
+                })}
+              

+            );
+          }
+        })}
+
+
, + ); +} diff --git a/gui/src/pages/gui/ToolCallDiv/FunctionSpecificToolCallDiv.tsx b/gui/src/pages/gui/ToolCallDiv/FunctionSpecificToolCallDiv.tsx index ac74802a25d..0fcdc0fc16e 100644 --- a/gui/src/pages/gui/ToolCallDiv/FunctionSpecificToolCallDiv.tsx +++ b/gui/src/pages/gui/ToolCallDiv/FunctionSpecificToolCallDiv.tsx @@ -1,7 +1,9 @@ import { ToolCallState } from "core"; import { BuiltInToolNames } from "core/tools/builtIn"; +import { EditOperation } from "core/tools/definitions/multiEdit"; import { CreateFile } from "./CreateFile"; import { EditFile } from "./EditFile"; +import { FindAndReplaceDisplay } from "./FindAndReplace"; import { RunTerminalCommand } from "./RunTerminalCommand"; function FunctionSpecificToolCallDiv({ @@ -50,40 +52,30 @@ function FunctionSpecificToolCallDiv({ /> ); case BuiltInToolNames.SingleFindAndReplace: - const fakeSearchReplaceBlock = `===== SEARCH -${args?.old_string ?? ""} -===== -${args?.new_string ?? ""} -===== REPLACE`; - + const edits: EditOperation[] = [ + { + old_string: args?.old_string ?? "", + new_string: args?.new_string ?? "", + replace_all: args?.replace_all, + }, + ]; return ( - ); case BuiltInToolNames.MultiEdit: - const fakeSearchReplaceBlocks = - (args?.edits as { old_string: string; new_string: string }[]) - ?.map( - (edit) => `===== SEARCH -${edit?.old_string ?? ""} -===== -${edit?.new_string ?? ""} -===== REPLACE`, - ) - ?.join("\n\n---\n\n") ?? ""; - return ( - diff --git a/gui/src/redux/slices/sessionSlice.ts b/gui/src/redux/slices/sessionSlice.ts index 3ec2fe27b87..7251b04ac58 100644 --- a/gui/src/redux/slices/sessionSlice.ts +++ b/gui/src/redux/slices/sessionSlice.ts @@ -849,6 +849,21 @@ export const sessionSlice = createSlice({ ); } }, + setToolCallArgs: ( + state, + action: PayloadAction<{ + toolCallId: string; + newArgs: Record; + }>, + ) => { + const toolCallState = findToolCallById( + state.history, + action.payload.toolCallId, + ); + if (toolCallState) { + toolCallState.parsedArgs = action.payload.newArgs; + } + }, cancelToolCall: ( state, action: PayloadAction<{ @@ -1024,6 +1039,7 @@ export const { acceptToolCall, setToolGenerated, updateToolCallOutput, + setToolCallArgs, setMode, setIsSessionMetadataLoading, setAllSessionMetadata, diff --git a/gui/src/redux/thunks/enhanceParsedArgs.ts b/gui/src/redux/thunks/enhanceParsedArgs.ts new file mode 100644 index 00000000000..75141abb8bc --- /dev/null +++ b/gui/src/redux/thunks/enhanceParsedArgs.ts @@ -0,0 +1,53 @@ +import { BuiltInToolNames } from "core/tools/builtIn"; +import { resolveRelativePathInDir } from "core/util/ideUtils"; +import { getUriPathBasename } from "core/util/uri"; +import { IIdeMessenger } from "../../context/IdeMessenger"; +import { setToolCallArgs } from "../slices/sessionSlice"; +import { AppThunkDispatch } from "../store"; + +export async function enhanceParsedArgs( + ideMessenger: IIdeMessenger, + dispatch: AppThunkDispatch, + toolName: string | undefined, + toolCallId: string, + currentArgs: undefined | Record, +) { + // Add file content to parsedArgs for find/replace tools + let enhancedArgs = { ...currentArgs }; + if ( + (toolName === BuiltInToolNames.SingleFindAndReplace || + toolName === BuiltInToolNames.MultiEdit || + toolName === BuiltInToolNames.SearchAndReplaceInFile) && + currentArgs?.filepath && + !currentArgs?.editingFileContents + ) { + try { + const fileUri = await resolveRelativePathInDir( + currentArgs.filepath, + ideMessenger.ide, + ); + if (!fileUri) { + throw new Error(`File ${currentArgs.filepath} not found`); + } + const baseName = getUriPathBasename(fileUri); + const fileContent = await ideMessenger.ide.readFile(fileUri); + enhancedArgs = { + ...currentArgs, + fileUri, + baseName, + editingFileContents: fileContent, + }; + dispatch( + setToolCallArgs({ + toolCallId, + newArgs: enhancedArgs, + }), + ); + } catch (error) { + // If we can't read the file, let the tool handle the error + console.warn( + `Failed to enhance args: failed to read file ${currentArgs?.filepath}`, + ); + } + } +} diff --git a/gui/src/redux/thunks/streamNormalInput.ts b/gui/src/redux/thunks/streamNormalInput.ts index b17cf3d2992..6adfaf19910 100644 --- a/gui/src/redux/thunks/streamNormalInput.ts +++ b/gui/src/redux/thunks/streamNormalInput.ts @@ -28,6 +28,7 @@ import { selectCurrentToolCalls } from "../selectors/selectToolCalls"; import { DEFAULT_TOOL_SETTING } from "../slices/uiSlice"; import { getBaseSystemMessage } from "../util/getBaseSystemMessage"; import { callToolById } from "./callToolById"; +import { enhanceParsedArgs } from "./enhanceParsedArgs"; /** * Handles the execution of tool calls that may be automatically accepted. * Sets all tools as generated first, then executes auto-approved tool calls. @@ -253,6 +254,20 @@ export const streamNormalInput = createAsyncThunk< const newState = getState(); const toolSettings = newState.ui.toolSettings; const allToolCallStates = selectCurrentToolCalls(newState); + + console.log("ADDING EDITNG CURRENT FILE"); + await Promise.all( + allToolCallStates.map((tcState) => + enhanceParsedArgs( + extra.ideMessenger, + dispatch, + tcState?.toolCall.function.name, + tcState.toolCallId, + tcState.parsedArgs, + ), + ), + ); + const generatingToolCalls = allToolCallStates.filter( (toolCallState) => toolCallState.status === "generating", ); diff --git a/gui/src/util/clientTools/multiEditImpl.ts b/gui/src/util/clientTools/multiEditImpl.ts index 66d1167231d..8b12109e5ad 100644 --- a/gui/src/util/clientTools/multiEditImpl.ts +++ b/gui/src/util/clientTools/multiEditImpl.ts @@ -16,7 +16,7 @@ export const multiEditImpl: ClientToolImpl = async ( toolCallId, extras, ) => { - const { filepath, edits } = args; + const { filepath, edits, editingFileContents } = args; const streamId = uuid(); @@ -64,7 +64,9 @@ export const multiEditImpl: ClientToolImpl = async ( `file ${filepath} does not exist. If you are trying to edit it, correct the filepath. If you are trying to create it, you must pass old_string=""`, ); } - newContent = await extras.ideMessenger.ide.readFile(resolvedUri); + newContent = + editingFileContents ?? + (await extras.ideMessenger.ide.readFile(resolvedUri)); fileUri = resolvedUri; for (let i = 0; i < edits.length; i++) { const { old_string, new_string, replace_all } = edits[i]; diff --git a/gui/src/util/clientTools/searchReplaceImpl.ts b/gui/src/util/clientTools/searchReplaceImpl.ts index ab931d40a28..5b750b26f57 100644 --- a/gui/src/util/clientTools/searchReplaceImpl.ts +++ b/gui/src/util/clientTools/searchReplaceImpl.ts @@ -11,7 +11,7 @@ export const searchReplaceToolImpl: ClientToolImpl = async ( toolCallId, extras, ) => { - const { filepath, diffs } = args; + const { filepath, diffs, editingFileContents } = args; const state = extras.getState(); const allowAnonymousTelemetry = state.config.config.allowAnonymousTelemetry; @@ -46,7 +46,8 @@ export const searchReplaceToolImpl: ClientToolImpl = async ( try { // Read the current file content const originalContent = - await extras.ideMessenger.ide.readFile(resolvedFilepath); + editingFileContents ?? + (await extras.ideMessenger.ide.readFile(resolvedFilepath)); let currentContent = originalContent; // Apply all replacements sequentially to build the final content diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts index aec0ebf816a..f3bfe96d869 100644 --- a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts @@ -12,7 +12,13 @@ export const singleFindAndReplaceImpl: ClientToolImpl = async ( toolCallId, extras, ) => { - const { filepath, old_string, new_string, replace_all = false } = args; + const { + filepath, + old_string, + new_string, + replace_all = false, + editingFileContents, + } = args; const streamId = uuid(); @@ -33,7 +39,8 @@ export const singleFindAndReplaceImpl: ClientToolImpl = async ( // Read the current file content const originalContent = - await extras.ideMessenger.ide.readFile(resolvedFilepath); + (await editingFileContents) ?? + extras.ideMessenger.ide.readFile(resolvedFilepath); // Perform the find and replace operation const newContent = performFindAndReplace( From 07c39309c18588f61b082ae3672539b1954043f3 Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Sat, 23 Aug 2025 16:58:25 -0700 Subject: [PATCH 11/14] test: find and replace ui tests --- .../gui/ToolCallDiv/FindAndReplace.test.tsx | 446 ++++++++++++------ .../pages/gui/ToolCallDiv/FindAndReplace.tsx | 9 +- .../clientTools/singleFindAndReplaceImpl.ts | 4 +- 3 files changed, 301 insertions(+), 158 deletions(-) diff --git a/gui/src/pages/gui/ToolCallDiv/FindAndReplace.test.tsx b/gui/src/pages/gui/ToolCallDiv/FindAndReplace.test.tsx index 3e912bd66a8..8cf92ba4c7d 100644 --- a/gui/src/pages/gui/ToolCallDiv/FindAndReplace.test.tsx +++ b/gui/src/pages/gui/ToolCallDiv/FindAndReplace.test.tsx @@ -1,209 +1,347 @@ -import { configureStore } from "@reduxjs/toolkit"; -import { render, screen } from "@testing-library/react"; +import { fireEvent, render, screen } from "@testing-library/react"; import { ApplyState } from "core"; -import { Provider } from "react-redux"; -import { describe, expect, it, Mock, vi } from "vitest"; -import { useFileContent } from "../../../hooks/useFileContent"; -import { SingleFindAndReplace } from "./FindAndReplace"; +import { EditOperation } from "core/tools/definitions/multiEdit"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { FindAndReplaceDisplay } from "./FindAndReplace"; -// Mock the useFileContent hook -vi.mock("../../../hooks/useFileContent"); +// Mock the dependencies +vi.mock("../../../context/IdeMessenger", () => ({ + IdeMessengerContext: { + _currentValue: { post: vi.fn() }, + }, +})); -// Mock the selectApplyStateByToolCallId selector -vi.mock("../../../redux/selectors/selectToolCalls", () => ({ - selectApplyStateByToolCallId: vi.fn(), +vi.mock("../../../redux/hooks", () => ({ + useAppSelector: vi.fn(), })); -const mockUseFileContent = useFileContent as Mock; +vi.mock("../../../components/ui", () => ({ + useFontSize: () => 14, +})); -// Create a minimal store for testing -const createTestStore = (applyState?: ApplyState) => { - return configureStore({ - reducer: { - session: () => ({ - codeBlockApplyStates: { - states: applyState ? [applyState] : [], - }, - }), - }, - }); -}; +vi.mock("../../../util/clientTools/findAndReplaceUtils", () => ({ + performFindAndReplace: vi.fn(), +})); + +vi.mock("./utils", () => ({ + getStatusIcon: vi.fn(() =>
✓
), +})); -describe("SingleFindAndReplace", () => { +vi.mock("react", async () => { + const actual = await vi.importActual("react"); + return { + ...actual, + useContext: () => ({ post: vi.fn() }), + }; +}); + +// Import mocked modules +import { useAppSelector } from "../../../redux/hooks"; +import { performFindAndReplace } from "../../../util/clientTools/findAndReplaceUtils"; + +const mockPost = vi.fn(); +const mockUseAppSelector = useAppSelector as any; +const mockPerformFindAndReplace = performFindAndReplace as any; + +describe("FindAndReplaceDisplay", () => { const defaultProps = { - relativeFilePath: "test.ts", - oldString: "old code", - newString: "new code", - replaceAll: false, + fileUri: "file:///test/file.ts", + relativeFilePath: "test/file.ts", + editingFileContents: "const old = 'value';\nconst other = 'test';", + edits: [ + { + old_string: "const old = 'value';", + new_string: "const new = 'value';", + replace_all: false, + }, + ] as EditOperation[], toolCallId: "test-tool-call-id", historyIndex: 0, }; + const mockToolCallState = { + status: "done" as const, + output: null, + }; + + const mockConfig = { + ui: { + showChatScrollbar: true, + codeWrap: false, + }, + }; + beforeEach(() => { vi.clearAllMocks(); - }); - it("shows loading state when file content is loading", () => { - mockUseFileContent.mockReturnValue({ - fileContent: null, - isLoading: true, - error: null, + mockUseAppSelector.mockImplementation((selector: any) => { + const mockState = { + config: { config: mockConfig }, + session: { + history: [], + codeBlockApplyStates: { states: [] }, + }, + }; + + if (selector.toString().includes("selectApplyStateByToolCallId")) { + return undefined; // No apply state by default + } + if (selector.toString().includes("selectToolCallById")) { + return mockToolCallState; + } + + return selector(mockState); }); - const store = createTestStore(); - render( - - - , + mockPerformFindAndReplace.mockImplementation( + (content: string, oldStr: string, newStr: string) => { + return content.replace(oldStr, newStr); + }, ); - - expect(screen.getByText("Loading file content...")).toBeInTheDocument(); }); - it("shows error when file content fails to load", () => { - mockUseFileContent.mockReturnValue({ - fileContent: null, - isLoading: false, - error: "File not found", + describe("basic rendering", () => { + it("should render with collapsed state by default", () => { + render(); + + expect(screen.getByText("file.ts")).toBeInTheDocument(); + expect( + screen.getByTestId("toggle-find-and-replace-diff"), + ).toBeInTheDocument(); + + // Should not show diff content when collapsed + const diffContent = screen.queryByText("-"); + expect(diffContent).not.toBeInTheDocument(); }); - const store = createTestStore(); - render( - - - , - ); + it("should display file name from fileUri", () => { + render(); + expect(screen.getByText("file.ts")).toBeInTheDocument(); + }); + + it("should display file name from relativeFilePath when no fileUri", () => { + render( + , + ); + expect(screen.getByText("test.tsx")).toBeInTheDocument(); + }); - expect( - screen.getByText(`Failed to load file: ${defaultProps.relativeFilePath}`), - ).toBeInTheDocument(); + it("should handle missing file paths gracefully", () => { + render( + , + ); + + // Should still render the component structure + expect( + screen.getByTestId("toggle-find-and-replace-diff"), + ).toBeInTheDocument(); + }); }); - it("shows error when find and replace operation fails", () => { - mockUseFileContent.mockReturnValue({ - fileContent: "some content without the old string", - isLoading: false, - error: null, + describe("expand/collapse functionality", () => { + it("should expand when clicked", () => { + render(); + + const toggleButton = screen.getByTestId("toggle-find-and-replace-diff"); + fireEvent.click(toggleButton); + + // Should show diff content when expanded + expect(screen.getByText("-")).toBeInTheDocument(); + expect(screen.getByText("+")).toBeInTheDocument(); }); - const store = createTestStore(); - render( - - - , - ); + it("should show content when tool call status is 'generated'", () => { + mockUseAppSelector.mockImplementation((selector: any) => { + const mockState = { + config: { config: mockConfig }, + session: { + history: [], + codeBlockApplyStates: { states: [] }, + }, + }; + + if (selector.toString().includes("selectToolCallById")) { + return { ...mockToolCallState, status: "generated" }; + } + if (selector.toString().includes("selectApplyStateByToolCallId")) { + return undefined; + } - expect(screen.getByText(/Error:/)).toBeInTheDocument(); - expect(screen.getByText(/String not found in file/)).toBeInTheDocument(); + return selector(mockState); + }); + + render(); + + // Should show diff content without expanding + expect(screen.getByText("-")).toBeInTheDocument(); + expect(screen.getByText("+")).toBeInTheDocument(); + }); }); - it("shows no changes message when old and new strings result in same content", () => { - mockUseFileContent.mockReturnValue({ - fileContent: "some content", - isLoading: false, - error: null, + describe("diff generation", () => { + it("should generate and display diff correctly", () => { + render(); + + const toggleButton = screen.getByTestId("toggle-find-and-replace-diff"); + fireEvent.click(toggleButton); + + // Should show removed line + expect(screen.getByText("const old = 'value';")).toBeInTheDocument(); + + // Should show added line + expect(screen.getByText("const new = 'value';")).toBeInTheDocument(); + + // Should show unchanged line + expect(screen.getByText("const other = 'test';")).toBeInTheDocument(); }); - const store = createTestStore(); - render( - - - , - ); + it("should handle multiple edits", () => { + const multipleEdits = [ + { + old_string: "const old = 'value';", + new_string: "const new = 'value';", + replace_all: false, + }, + { + old_string: "const other = 'test';", + new_string: "const other = 'updated';", + replace_all: false, + }, + ] as EditOperation[]; - // This should show an error since the string doesn't exist - expect(screen.getByText(/Error:/)).toBeInTheDocument(); - }); + mockPerformFindAndReplace + .mockReturnValueOnce("const new = 'value';\nconst other = 'test';") + .mockReturnValueOnce("const new = 'value';\nconst other = 'updated';"); + + render(); - it("shows diff when find and replace operation succeeds", () => { - const fileContent = "Hello old code world"; - mockUseFileContent.mockReturnValue({ - fileContent, - isLoading: false, - error: null, + const toggleButton = screen.getByTestId("toggle-find-and-replace-diff"); + fireEvent.click(toggleButton); + + expect(mockPerformFindAndReplace).toHaveBeenCalledTimes(2); }); - const store = createTestStore(); - render( - - - , - ); + it("should handle diff generation errors", () => { + mockPerformFindAndReplace.mockImplementation(() => { + throw new Error("Test error"); + }); - expect(screen.getByText(defaultProps.relativeFilePath)).toBeInTheDocument(); - // The diff should show the old line being removed and new line being added - expect(screen.getByText("Hello old code world")).toBeInTheDocument(); - expect(screen.getByText("Hello new code world")).toBeInTheDocument(); - }); + render(); + + const toggleButton = screen.getByTestId("toggle-find-and-replace-diff"); + fireEvent.click(toggleButton); - it("shows apply state status when available", () => { - mockUseFileContent.mockReturnValue({ - fileContent: "Hello old code world", - isLoading: false, - error: null, + expect(screen.getByText("Error generating diff")).toBeInTheDocument(); + expect(screen.getByText("Test error")).toBeInTheDocument(); }); - const applyState: ApplyState = { - streamId: "test-stream", + it("should show 'No changes to display' when diff is empty", () => { + // Mock the function to return the exact same content (no changes) + mockPerformFindAndReplace.mockReturnValue( + defaultProps.editingFileContents, + ); + + render(); + + const toggleButton = screen.getByTestId("toggle-find-and-replace-diff"); + fireEvent.click(toggleButton); + + expect(screen.getByText("No changes to display")).toBeInTheDocument(); + }); + }); + + describe("apply actions", () => { + const mockApplyState: ApplyState = { + streamId: "test-stream-id", status: "streaming", - toolCallId: "test-tool-call-id", + numDiffs: 1, + fileContent: "test content", }; - const store = createTestStore(applyState); - render( - - - , - ); + beforeEach(() => { + mockUseAppSelector.mockImplementation((selector: any) => { + const mockState = { + config: { config: mockConfig }, + session: { + history: [], + codeBlockApplyStates: { states: [mockApplyState] }, + }, + }; - expect(screen.getByText("Applying...")).toBeInTheDocument(); - }); + if (selector.toString().includes("selectApplyStateByToolCallId")) { + return mockApplyState; + } + if (selector.toString().includes("selectToolCallById")) { + return mockToolCallState; + } - it("shows tool call status icon when enabled", () => { - mockUseFileContent.mockReturnValue({ - fileContent: "Hello old code world", - isLoading: false, - error: null, + return selector(mockState); + }); }); - const store = createTestStore(); - render( - - - , - ); + it("should show apply actions when applyState exists", () => { + render(); + + // ApplyActions component should be rendered (we can test by looking for its structure) + // Since we don't have the exact structure, we test that the container is there + expect( + screen.getByTestId("toggle-find-and-replace-diff"), + ).toBeInTheDocument(); + }); + + it("should handle accept action", () => { + render(); - // Look for the status indicator (green dot for done status) - const statusIndicator = document.querySelector(".bg-green-500"); - expect(statusIndicator).toBeInTheDocument(); + // This would need more detailed testing if ApplyActions was properly mocked + // For now, we verify the component renders without errors + expect( + screen.getByTestId("toggle-find-and-replace-diff"), + ).toBeInTheDocument(); + }); }); - it("handles replace all option correctly", () => { - const fileContent = "old code and more old code"; - mockUseFileContent.mockReturnValue({ - fileContent, - isLoading: false, - error: null, + describe("content handling", () => { + it("should use editingFileContents when provided", () => { + render(); + + const toggleButton = screen.getByTestId("toggle-find-and-replace-diff"); + fireEvent.click(toggleButton); + + expect(mockPerformFindAndReplace).toHaveBeenCalledWith( + "const old = 'value';\nconst other = 'test';", + "const old = 'value';", + "const new = 'value';", + false, + 0, + ); }); - const store = createTestStore(); - render( - - - , - ); + it("should fallback to edit old_strings when no editingFileContents", () => { + render( + , + ); - // Should show both replacements in the diff - expect(screen.getByText(defaultProps.relativeFilePath)).toBeInTheDocument(); - expect(screen.getByText("old code and more old code")).toBeInTheDocument(); - expect(screen.getByText("new code and more new code")).toBeInTheDocument(); + const toggleButton = screen.getByTestId("toggle-find-and-replace-diff"); + fireEvent.click(toggleButton); + + expect(mockPerformFindAndReplace).toHaveBeenCalledWith( + "const old = 'value';", + "const old = 'value';", + "const new = 'value';", + false, + 0, + ); + }); }); }); diff --git a/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx b/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx index 55cf27ae31e..f13ad6855e3 100644 --- a/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx +++ b/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx @@ -60,7 +60,7 @@ export function FindAndReplaceDisplay({ if (editingFileContents) { return editingFileContents; } - return edits.map((edit) => edit.old_string).join("\n"); + return edits?.map((edit) => edit.old_string ?? "").join("\n"); }, [editingFileContents, edits]); const diffResult = useMemo(() => { @@ -182,7 +182,12 @@ export function FindAndReplaceDisplay({ ); } - if (!diffResult?.diff) { + if ( + !diffResult?.diff || + (diffResult.diff.length === 1 && + !diffResult.diff[0].added && + !diffResult.diff[0].removed) + ) { return renderContainer(
No changes to display
, ); diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts index f3bfe96d869..92c3da17838 100644 --- a/gui/src/util/clientTools/singleFindAndReplaceImpl.ts +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.ts @@ -39,8 +39,8 @@ export const singleFindAndReplaceImpl: ClientToolImpl = async ( // Read the current file content const originalContent = - (await editingFileContents) ?? - extras.ideMessenger.ide.readFile(resolvedFilepath); + editingFileContents ?? + (await extras.ideMessenger.ide.readFile(resolvedFilepath)); // Perform the find and replace operation const newContent = performFindAndReplace( From 0208969b856579173d381707795cdd84c7b5e13c Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Sat, 23 Aug 2025 18:31:08 -0700 Subject: [PATCH 12/14] chore: cleanup console logs --- gui/src/redux/thunks/streamNormalInput.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/gui/src/redux/thunks/streamNormalInput.ts b/gui/src/redux/thunks/streamNormalInput.ts index 6adfaf19910..dbd322357d8 100644 --- a/gui/src/redux/thunks/streamNormalInput.ts +++ b/gui/src/redux/thunks/streamNormalInput.ts @@ -255,7 +255,6 @@ export const streamNormalInput = createAsyncThunk< const toolSettings = newState.ui.toolSettings; const allToolCallStates = selectCurrentToolCalls(newState); - console.log("ADDING EDITNG CURRENT FILE"); await Promise.all( allToolCallStates.map((tcState) => enhanceParsedArgs( From 56b7195c712403331c2a8a9b43d43e06915023f1 Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Mon, 25 Aug 2025 12:34:54 -0700 Subject: [PATCH 13/14] fix: cubic feedback --- gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx b/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx index f13ad6855e3..18d995601cd 100644 --- a/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx +++ b/gui/src/pages/gui/ToolCallDiv/FindAndReplace.tsx @@ -120,7 +120,7 @@ export function FindAndReplaceDisplay({ ); } } - }, [toolCallState?.status]); + }, [toolCallState?.status, toolCallState?.output]); // Unified container component that always renders the same structure const renderContainer = (content: React.ReactNode) => ( @@ -205,7 +205,7 @@ export function FindAndReplaceDisplay({ return (
{part.value.split("\n").map((line, lineIndex) => { if ( @@ -226,7 +226,7 @@ export function FindAndReplaceDisplay({ return (
{part.value.split("\n").map((line, lineIndex) => { if ( From fc93a2e922d58b66a28a52ddf48812b2a98f70f5 Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Mon, 25 Aug 2025 14:12:54 -0700 Subject: [PATCH 14/14] fix: make search and replace edit copy less confusing --- .../clientTools/findAndReplaceUtils.test.ts | 30 +++++++++---------- .../util/clientTools/findAndReplaceUtils.ts | 15 ++++++---- .../util/clientTools/multiEditImpl.test.ts | 15 ++++++---- .../singleFindAndReplaceImpl.test.ts | 2 +- 4 files changed, 35 insertions(+), 27 deletions(-) diff --git a/gui/src/util/clientTools/findAndReplaceUtils.test.ts b/gui/src/util/clientTools/findAndReplaceUtils.test.ts index 53d541a8362..68ab333b9a3 100644 --- a/gui/src/util/clientTools/findAndReplaceUtils.test.ts +++ b/gui/src/util/clientTools/findAndReplaceUtils.test.ts @@ -1,6 +1,8 @@ import { EditOperation } from "core/tools/definitions/multiEdit"; import { describe, expect, it } from "vitest"; import { + EMPTY_NON_FIRST_EDIT_MESSAGE, + FOUND_MULTIPLE_FIND_STRINGS_ERROR, performFindAndReplace, validateCreatingForMultiEdit, validateSingleEdit, @@ -152,7 +154,7 @@ describe("performFindAndReplace", () => { expect(() => { performFindAndReplace(content, "xyz", "abc"); - }).toThrow('String not found in file: "xyz"'); + }).toThrow('string not found in file: "xyz"'); }); it("should throw error with edit context when string not found", () => { @@ -160,7 +162,7 @@ describe("performFindAndReplace", () => { expect(() => { performFindAndReplace(content, "xyz", "abc", false, 2); - }).toThrow('Edit #3: String not found in file: "xyz"'); + }).toThrow('edit at index 2: string not found in file: "xyz"'); }); it("should throw error when multiple occurrences exist and replaceAll is false", () => { @@ -169,7 +171,7 @@ describe("performFindAndReplace", () => { expect(() => { performFindAndReplace(content, "Hello", "Hi", false); }).toThrow( - 'String "Hello" appears 3 times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.', + `String "Hello" appears 3 times in the file. ${FOUND_MULTIPLE_FIND_STRINGS_ERROR}`, ); }); @@ -179,7 +181,7 @@ describe("performFindAndReplace", () => { expect(() => { performFindAndReplace(content, "test", "exam", false, 1); }).toThrow( - 'Edit #2: String "test" appears 3 times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.', + `edit at index 1: String "test" appears 3 times in the file. ${FOUND_MULTIPLE_FIND_STRINGS_ERROR}`, ); }); }); @@ -296,19 +298,21 @@ describe("validateSingleEdit", () => { it("should include edit number in error messages when index provided", () => { expect(() => { validateSingleEdit(null as any, "new", 2); - }).toThrow("Edit #3: old_string is required"); + }).toThrow("edit at index 2: old_string is required"); }); it("should include edit number for new_string errors", () => { expect(() => { validateSingleEdit("old", undefined as any, 0); - }).toThrow("Edit #1: new_string is required"); + }).toThrow("edit at index 0: new_string is required"); }); it("should include edit number for identical strings error", () => { expect(() => { validateSingleEdit("same", "same", 4); - }).toThrow("Edit #5: old_string and new_string must be different"); + }).toThrow( + "edit at index 4: old_string and new_string must be different", + ); }); it("should not include context when index not provided", () => { @@ -382,9 +386,7 @@ describe("validateCreatingForMultiEdit", () => { expect(() => { validateCreatingForMultiEdit(edits); - }).toThrow( - "edit #2: only the first edit can contain an empty old_string, which is only used for file creation.", - ); + }).toThrow(`edit at index 1: ${EMPTY_NON_FIRST_EDIT_MESSAGE}`); }); it("should throw error with correct edit number for empty old_string", () => { @@ -396,9 +398,7 @@ describe("validateCreatingForMultiEdit", () => { expect(() => { validateCreatingForMultiEdit(edits); - }).toThrow( - "edit #3: only the first edit can contain an empty old_string, which is only used for file creation.", - ); + }).toThrow(`edit at index 2: ${EMPTY_NON_FIRST_EDIT_MESSAGE}`); }); it("should throw error for multiple empty old_strings in subsequent edits", () => { @@ -410,9 +410,7 @@ describe("validateCreatingForMultiEdit", () => { expect(() => { validateCreatingForMultiEdit(edits); - }).toThrow( - "edit #2: only the first edit can contain an empty old_string, which is only used for file creation.", - ); + }).toThrow(`edit at index 1: ${EMPTY_NON_FIRST_EDIT_MESSAGE}`); }); }); diff --git a/gui/src/util/clientTools/findAndReplaceUtils.ts b/gui/src/util/clientTools/findAndReplaceUtils.ts index b10ce6adc9d..6828e1695b4 100644 --- a/gui/src/util/clientTools/findAndReplaceUtils.ts +++ b/gui/src/util/clientTools/findAndReplaceUtils.ts @@ -1,5 +1,8 @@ import { EditOperation } from "core/tools/definitions/multiEdit"; +export const FOUND_MULTIPLE_FIND_STRINGS_ERROR = + "Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences."; + /** * Performs a find and replace operation on text content with proper handling of special characters */ @@ -10,10 +13,10 @@ export function performFindAndReplace( replaceAll: boolean = false, index?: number, // For error messages ): string { - const errorContext = index !== undefined ? `Edit #${index + 1}: ` : ""; + const errorContext = index !== undefined ? `edit at index ${index}: ` : ""; // Check if old_string exists in current content if (!content.includes(oldString)) { - throw new Error(`${errorContext}String not found in file: "${oldString}"`); + throw new Error(`${errorContext}string not found in file: "${oldString}"`); } if (replaceAll) { @@ -35,7 +38,7 @@ export function performFindAndReplace( if (count > 1) { throw new Error( - `${errorContext}String "${oldString}" appears ${count} times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.`, + `${errorContext}String "${oldString}" appears ${count} times in the file. ${FOUND_MULTIPLE_FIND_STRINGS_ERROR}`, ); } @@ -57,7 +60,7 @@ export function validateSingleEdit( newString: string, index?: number, ): void { - const context = index !== undefined ? `Edit #${index + 1}: ` : ""; + const context = index !== undefined ? `edit at index ${index}: ` : ""; if (!oldString && oldString !== "") { throw new Error(`${context}old_string is required`); @@ -70,6 +73,8 @@ export function validateSingleEdit( } } +export const EMPTY_NON_FIRST_EDIT_MESSAGE = + "contains empty old_string. Only the first edit can contain an empty old_string, which is only used for file creation."; export function validateCreatingForMultiEdit(edits: EditOperation[]) { const isCreating = edits[0].old_string === ""; if (edits.length > 1) { @@ -81,7 +86,7 @@ export function validateCreatingForMultiEdit(edits: EditOperation[]) { for (let i = 1; i < edits.length; i++) { if (edits[i].old_string === "") { throw new Error( - `edit #${i + 1}: only the first edit can contain an empty old_string, which is only used for file creation.`, + `edit at index ${i}: ${EMPTY_NON_FIRST_EDIT_MESSAGE}`, ); } } diff --git a/gui/src/util/clientTools/multiEditImpl.test.ts b/gui/src/util/clientTools/multiEditImpl.test.ts index e98ff7ec1ed..7c262cb8246 100644 --- a/gui/src/util/clientTools/multiEditImpl.test.ts +++ b/gui/src/util/clientTools/multiEditImpl.test.ts @@ -2,6 +2,7 @@ import * as ideUtils from "core/util/ideUtils"; import { beforeEach, describe, expect, it, Mock, vi } from "vitest"; import { applyForEditTool } from "../../redux/thunks/handleApplyStateUpdate"; import { ClientToolExtras } from "./callClientTool"; +import { FOUND_MULTIPLE_FIND_STRINGS_ERROR } from "./findAndReplaceUtils"; import { multiEditImpl } from "./multiEditImpl"; vi.mock("uuid", () => ({ @@ -72,7 +73,7 @@ describe("multiEditImpl", () => { "id", mockExtras, ), - ).rejects.toThrow("Edit #1: old_string is required"); + ).rejects.toThrow("edit at index 0: old_string is required"); }); it("should throw if edit has undefined new_string", async () => { @@ -85,7 +86,7 @@ describe("multiEditImpl", () => { "id", mockExtras, ), - ).rejects.toThrow("Edit #1: new_string is required"); + ).rejects.toThrow("edit at index 0: new_string is required"); }); it("should throw if old_string equals new_string", async () => { @@ -98,7 +99,9 @@ describe("multiEditImpl", () => { "id", mockExtras, ), - ).rejects.toThrow("Edit #1: old_string and new_string must be different"); + ).rejects.toThrow( + "edit at index 0: old_string and new_string must be different", + ); }); }); @@ -201,7 +204,9 @@ describe("multiEditImpl", () => { "id", mockExtras, ), - ).rejects.toThrow('Edit #2: String not found in file: "not found"'); + ).rejects.toThrow( + 'edit at index 1: string not found in file: "not found"', + ); }); it("should throw if multiple occurrences without replace_all", async () => { @@ -219,7 +224,7 @@ describe("multiEditImpl", () => { mockExtras, ), ).rejects.toThrow( - 'Edit #1: String "test" appears 3 times in the file. Either provide a more specific string with surrounding context to make it unique, or use replace_all=true to replace all occurrences.', + `edit at index 0: String "test" appears 3 times in the file. ${FOUND_MULTIPLE_FIND_STRINGS_ERROR}`, ); }); }); diff --git a/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts b/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts index 3606a0b4050..e6e8201b9f2 100644 --- a/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts +++ b/gui/src/util/clientTools/singleFindAndReplaceImpl.test.ts @@ -148,7 +148,7 @@ describe("singleFindAndReplaceImpl", () => { await expect( singleFindAndReplaceImpl(args, "tool-call-id", mockExtras), - ).rejects.toThrow(`String not found in file: "not found"`); + ).rejects.toThrow(`string not found in file: "not found"`); }); it("should replace single occurrence", async () => {