diff --git a/src/tests/edit-tool.test.ts b/src/tests/edit-tool.test.ts new file mode 100644 index 0000000..ffc91e6 --- /dev/null +++ b/src/tests/edit-tool.test.ts @@ -0,0 +1,304 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { promises as fs } from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { edit, unescapeOverEscapedString } from "../tools/impl/Edit"; + +describe("unescapeOverEscapedString", () => { + test("handles normal string without escapes", () => { + const input = "hello world"; + expect(unescapeOverEscapedString(input)).toBe("hello world"); + }); + + test("handles already correct escapes", () => { + // A string with proper escapes should pass through + const input = "line1\nline2"; + expect(unescapeOverEscapedString(input)).toBe("line1\nline2"); + }); + + test("fixes over-escaped newlines (\\\\n -> \\n)", () => { + // Input has literal backslash + n that should become newline + const input = "line1\\nline2"; + expect(unescapeOverEscapedString(input)).toBe("line1\nline2"); + }); + + test("fixes over-escaped tabs (\\\\t -> \\t)", () => { + const input = "col1\\tcol2"; + expect(unescapeOverEscapedString(input)).toBe("col1\tcol2"); + }); + + test("fixes over-escaped quotes", () => { + const input = 'say \\"hello\\"'; + expect(unescapeOverEscapedString(input)).toBe('say "hello"'); + }); + + test("fixes over-escaped backticks", () => { + const input = "template \\`literal\\`"; + expect(unescapeOverEscapedString(input)).toBe("template `literal`"); + }); + + test("fixes over-escaped single quotes", () => { + const input = "it\\'s working"; + expect(unescapeOverEscapedString(input)).toBe("it's working"); + }); + + test("fixes over-escaped backslashes before other escapes", () => { + // \\\\n (double-escaped newline) should become \n + const input = "line1\\\\nline2"; + expect(unescapeOverEscapedString(input)).toBe("line1\nline2"); + }); + + test("handles multiple over-escaped sequences", () => { + const input = "line1\\nline2\\n\\ttabbed\\nwith \\'quotes\\'"; + expect(unescapeOverEscapedString(input)).toBe( + "line1\nline2\n\ttabbed\nwith 'quotes'", + ); + }); + + test("handles over-escaped backticks in template literals", () => { + // LLMs often over-escape backticks - we fix those + const input = "const msg = \\`Hello World\\`;"; + expect(unescapeOverEscapedString(input)).toBe("const msg = `Hello World`;"); + }); + + test("preserves \\$ to avoid over-correcting shell/regex contexts", () => { + // \$ should NOT be unescaped - it's often intentional in shell scripts + const input = "echo \\$HOME"; + expect(unescapeOverEscapedString(input)).toBe("echo \\$HOME"); + }); +}); + +describe("edit tool", () => { + let tempDir: string; + let testFile: string; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "edit-test-")); + testFile = path.join(tempDir, "test.txt"); + }); + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + test("basic replacement works", async () => { + await fs.writeFile(testFile, "Hello World"); + + const result = await edit({ + file_path: testFile, + old_string: "World", + new_string: "Universe", + }); + + expect(result.replacements).toBe(1); + const content = await fs.readFile(testFile, "utf-8"); + expect(content).toBe("Hello Universe"); + }); + + test("multiline replacement works", async () => { + const original = `function hello() { + console.log("Hello"); +}`; + await fs.writeFile(testFile, original); + + const result = await edit({ + file_path: testFile, + old_string: 'console.log("Hello");', + new_string: 'console.log("Hello World");', + }); + + expect(result.replacements).toBe(1); + const content = await fs.readFile(testFile, "utf-8"); + expect(content).toContain('console.log("Hello World");'); + }); + + test("falls back to unescaping when direct match fails", async () => { + const original = "line1\nline2\nline3"; + await fs.writeFile(testFile, original); + + const result = await edit({ + file_path: testFile, + old_string: "line1\\nline2", + new_string: "replaced", + }); + + expect(result.replacements).toBe(1); + const content = await fs.readFile(testFile, "utf-8"); + expect(content).toBe("replaced\nline3"); + }); + + test("preserves new_string as-is even when old_string needed unescaping", async () => { + const original = "line1\nline2\nline3"; + await fs.writeFile(testFile, original); + + const result = await edit({ + file_path: testFile, + old_string: "line1\\nline2", + new_string: "keep\\nliteral", + }); + + expect(result.replacements).toBe(1); + const content = await fs.readFile(testFile, "utf-8"); + expect(content).toBe("keep\\nliteral\nline3"); + }); + + test("handles over-escaped backticks", async () => { + const original = "const msg = `Hello World`;"; + await fs.writeFile(testFile, original); + + const result = await edit({ + file_path: testFile, + old_string: "const msg = \\`Hello World\\`;", + new_string: "const msg = `Hi World`;", + }); + + expect(result.replacements).toBe(1); + const content = await fs.readFile(testFile, "utf-8"); + expect(content).toBe("const msg = `Hi World`;"); + }); + + test("normalizes CRLF in old_string to match LF in file", async () => { + const original = "line1\nline2\nline3"; + await fs.writeFile(testFile, original); + + const result = await edit({ + file_path: testFile, + old_string: "line1\r\nline2", + new_string: "lineA\nlineB", + }); + + expect(result.replacements).toBe(1); + const content = await fs.readFile(testFile, "utf-8"); + expect(content).toBe("lineA\nlineB\nline3"); + }); + + test("normalizes CRLF in file to match LF in old_string", async () => { + const original = "line1\r\nline2\r\nline3"; + await fs.writeFile(testFile, original); + + const result = await edit({ + file_path: testFile, + old_string: "line1\nline2", + new_string: "lineA\nlineB", + }); + + expect(result.replacements).toBe(1); + const content = await fs.readFile(testFile, "utf-8"); + expect(content).toBe("lineA\nlineB\nline3"); + }); + + test("replace_all works with fallback unescaping", async () => { + const original = "item\nitem\nitem"; + await fs.writeFile(testFile, original); + + const result = await edit({ + file_path: testFile, + old_string: "item", + new_string: "thing", + replace_all: true, + }); + + expect(result.replacements).toBe(3); + const content = await fs.readFile(testFile, "utf-8"); + expect(content).toBe("thing\nthing\nthing"); + }); + + test("uses expected_replacements as a safety check", async () => { + await fs.writeFile(testFile, "foo bar foo"); + + await expect( + edit({ + file_path: testFile, + old_string: "foo", + new_string: "qux", + expected_replacements: 1, + }), + ).rejects.toThrow("Expected 1 occurrence but found 2"); + }); + + test("replaces all when expected_replacements > 1", async () => { + await fs.writeFile(testFile, "foo bar foo"); + + const result = await edit({ + file_path: testFile, + old_string: "foo", + new_string: "qux", + expected_replacements: 2, + }); + + expect(result.replacements).toBe(2); + const content = await fs.readFile(testFile, "utf-8"); + expect(content).toBe("qux bar qux"); + }); + + test("throws error for invalid expected_replacements", async () => { + await fs.writeFile(testFile, "foo"); + + await expect( + edit({ + file_path: testFile, + old_string: "foo", + new_string: "bar", + expected_replacements: 0, + }), + ).rejects.toThrow("expected_replacements must be a positive integer"); + }); + + test("throws error when old_string is empty", async () => { + await fs.writeFile(testFile, "Hello World"); + + await expect( + edit({ + file_path: testFile, + old_string: "", + new_string: "Hello", + }), + ).rejects.toThrow("old_string cannot be empty"); + }); + + test("reports a smart-quote mismatch hint when applicable", async () => { + await fs.writeFile(testFile, "I\u2019ll be there."); + + await expect( + edit({ + file_path: testFile, + old_string: "I'll be there.", + new_string: "I will be there.", + }), + ).rejects.toThrow("Quote characters may differ"); + }); + + test("throws error when string not found even after unescaping", async () => { + await fs.writeFile(testFile, "Hello World"); + + await expect( + edit({ + file_path: testFile, + old_string: "Nonexistent", + new_string: "Replacement", + }), + ).rejects.toThrow("String to replace not found in file"); + }); + + test("throws error when old_string equals new_string", async () => { + await fs.writeFile(testFile, "Hello World"); + + await expect( + edit({ + file_path: testFile, + old_string: "Hello", + new_string: "Hello", + }), + ).rejects.toThrow("old_string and new_string are exactly the same"); + }); + + test("throws error for nonexistent file", async () => { + await expect( + edit({ + file_path: path.join(tempDir, "nonexistent.txt"), + old_string: "foo", + new_string: "bar", + }), + ).rejects.toThrow("File does not exist"); + }); +}); diff --git a/src/tests/tools/edit.test.ts b/src/tests/tools/edit.test.ts index fae46a8..6013007 100644 --- a/src/tests/tools/edit.test.ts +++ b/src/tests/tools/edit.test.ts @@ -66,6 +66,62 @@ describe("Edit tool", () => { expect(result.replacements).toBe(3); }); + test("uses expected_replacements as a safety check", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("duplicate.txt", "foo bar foo baz"); + + await expect( + edit({ + file_path: file, + old_string: "foo", + new_string: "qux", + expected_replacements: 1, + }), + ).rejects.toThrow("Expected 1 occurrence but found 2"); + }); + + test("replaces all when expected_replacements > 1", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("duplicate.txt", "foo bar foo baz"); + + const result = await edit({ + file_path: file, + old_string: "foo", + new_string: "qux", + expected_replacements: 2, + }); + + expect(readFileSync(file, "utf-8")).toBe("qux bar qux baz"); + expect(result.replacements).toBe(2); + }); + + test("throws error for invalid expected_replacements", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("test.txt", "foo"); + + await expect( + edit({ + file_path: file, + old_string: "foo", + new_string: "bar", + expected_replacements: 0, + }), + ).rejects.toThrow("expected_replacements must be a positive integer"); + }); + + test("throws error when old_string is empty", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("test.txt", "Hello, World!"); + + await expect( + edit({ + file_path: file, + old_string: "", + new_string: "Bun", + }), + ).rejects.toThrow("old_string cannot be empty"); + }); + test("throws error when file_path is missing", async () => { await expect( edit({ @@ -114,18 +170,15 @@ describe("Edit tool", () => { test("handles CRLF line endings (Windows compatibility)", async () => { testDir = new TestDirectory(); - // Create file with CRLF line endings (Windows style) const file = testDir.createFile("crlf.txt", ""); writeFileSync(file, "line1\r\nline2\r\nline3\r\n", "utf-8"); - // Edit with LF line endings (what the model typically sends) const result = await edit({ file_path: file, old_string: "line1\nline2", new_string: "changed1\nchanged2", }); - // Should successfully find and replace despite CRLF vs LF mismatch expect(result.replacements).toBe(1); const content = readFileSync(file, "utf-8"); expect(content).toContain("changed1"); @@ -135,10 +188,8 @@ describe("Edit tool", () => { test("handles mixed line endings", async () => { testDir = new TestDirectory(); const file = testDir.createFile("mixed.txt", ""); - // File with CRLF writeFileSync(file, "function foo() {\r\n return 1;\r\n}\r\n", "utf-8"); - // Model sends LF const result = await edit({ file_path: file, old_string: "function foo() {\n return 1;\n}", diff --git a/src/tests/tools/replace.test.ts b/src/tests/tools/replace.test.ts index 8c92b8c..7f05055 100644 --- a/src/tests/tools/replace.test.ts +++ b/src/tests/tools/replace.test.ts @@ -37,20 +37,17 @@ describe("Replace tool (Gemini)", () => { expect(readFileSync(filePath, "utf-8")).toBe("qux bar qux baz"); }); - test("creates new file when old_string is empty", async () => { + test("throws error when old_string is empty", async () => { testDir = new TestDirectory(); const filePath = testDir.resolve("new.txt"); - // Gemini's replace with empty old_string creates a new file - // But our Edit tool requires the file to exist, so this should throw - // Skip this test or use write_file_gemini instead await expect( replace({ file_path: filePath, old_string: "", new_string: "New content", }), - ).rejects.toThrow(/does not exist/); + ).rejects.toThrow("old_string cannot be empty"); }); test("throws error when file not found with non-empty old_string", async () => { diff --git a/src/tools/impl/Edit.ts b/src/tools/impl/Edit.ts index ed35a55..4a5d1b2 100644 --- a/src/tools/impl/Edit.ts +++ b/src/tools/impl/Edit.ts @@ -7,19 +7,137 @@ interface EditArgs { old_string: string; new_string: string; replace_all?: boolean; + expected_replacements?: number; } interface EditResult { message: string; replacements: number; } +function countOccurrences(content: string, needle: string): number { + if (needle.length === 0) { + return 0; + } + return content.split(needle).length - 1; +} + +function hasSmartQuoteMismatch(content: string, oldString: string): boolean { + const withRightSingle = oldString.replace(/'/g, "\u2019"); + const withLeftDouble = oldString.replace(/"/g, "\u201C"); + const withRightDouble = oldString.replace(/"/g, "\u201D"); + if (withRightSingle !== oldString && content.includes(withRightSingle)) { + return true; + } + if (withLeftDouble !== oldString && content.includes(withLeftDouble)) { + return true; + } + if (withRightDouble !== oldString && content.includes(withRightDouble)) { + return true; + } + return false; +} + +function buildNotFoundError( + originalOldString: string, + normalizedOldString: string, + content: string, +): Error { + const hints: string[] = []; + const trimmed = normalizedOldString.trim(); + if ( + trimmed !== normalizedOldString && + countOccurrences(content, trimmed) > 0 + ) { + hints.push("Leading or trailing whitespace differs from the file."); + } + if (hasSmartQuoteMismatch(content, normalizedOldString)) { + hints.push("Quote characters may differ (straight vs smart quotes)."); + } + const oldCollapsed = normalizedOldString.replace(/\s+/g, " ").trim(); + const contentCollapsed = content.replace(/\s+/g, " "); + if ( + oldCollapsed.length >= 20 && + oldCollapsed !== normalizedOldString && + contentCollapsed.includes(oldCollapsed) + ) { + hints.push("Line breaks or indentation may not match exactly."); + } + if (hints.length === 0) { + hints.push( + "The snippet may be stale; re-read the file and copy exact text.", + ); + } + + return new Error( + `String to replace not found in file.\nString: ${originalOldString}\nPossible mismatch reasons:\n- ${hints.join("\n- ")}`, + ); +} + +/** + * Unescapes a string that might have been overly escaped by an LLM. + * Based on Gemini CLI's unescapeStringForGeminiBug function. + * + * LLMs sometimes generate strings with extra escape characters like: + * - \\n instead of \n (newline) + * - \\t instead of \t (tab) + * - \\\" instead of " (quote) + * - \\` instead of ` (backtick) + */ +export function unescapeOverEscapedString(input: string): string { + // Match one or more backslashes followed by an escapable character + // and reduce to the proper single escape sequence. + // Based on Gemini CLI's unescapeStringForGeminiBug - intentionally conservative + // to avoid over-correcting intentional escapes in shell/regex contexts. + return input.replace( + /\\+(n|t|r|'|"|`|\\|\n)/g, + (_match: string, capturedChar: string): string => { + switch (capturedChar) { + case "n": + return "\n"; + case "t": + return "\t"; + case "r": + return "\r"; + case "'": + return "'"; + case '"': + return '"'; + case "`": + return "`"; + case "\\": + return "\\"; + case "\n": + return "\n"; + default: + return _match; + } + }, + ); +} + export async function edit(args: EditArgs): Promise { validateRequiredParams( args, ["file_path", "old_string", "new_string"], "Edit", ); - const { file_path, old_string, new_string, replace_all = false } = args; + const { file_path, replace_all = false, expected_replacements } = args; + // Normalize line endings in old_string and new_string to match file normalization + const old_string = args.old_string.replace(/\r\n/g, "\n"); + const new_string = args.new_string.replace(/\r\n/g, "\n"); + if (old_string.length === 0) { + throw new Error( + "old_string cannot be empty. Provide the exact text you want to replace.", + ); + } + if ( + expected_replacements !== undefined && + (!Number.isInteger(expected_replacements) || expected_replacements < 1) + ) { + throw new Error( + "expected_replacements must be a positive integer when provided.", + ); + } const userCwd = process.env.USER_CWD || process.cwd(); const resolvedPath = path.isAbsolute(file_path) ? file_path @@ -32,24 +150,51 @@ export async function edit(args: EditArgs): Promise { const rawContent = await fs.readFile(resolvedPath, "utf-8"); // Normalize line endings to LF for consistent matching (Windows uses CRLF) const content = rawContent.replace(/\r\n/g, "\n"); - const occurrences = content.split(old_string).length - 1; + let occurrences = countOccurrences(content, old_string); + let finalOldString = old_string; + const finalNewString = new_string; + + // If no match found, try unescaping old_string in case LLM over-escaped it + if (occurrences === 0) { + const unescapedOld = unescapeOverEscapedString(old_string); + const unescapedOccurrences = countOccurrences(content, unescapedOld); + + if (unescapedOccurrences > 0) { + // Unescaping old_string worked - use it for matching + // NOTE: We intentionally do NOT unescape new_string here. + // The user's replacement text should be used as-is. If they want + // actual newlines, they should provide actual newlines. + finalOldString = unescapedOld; + occurrences = unescapedOccurrences; + } + } + if (occurrences === 0) + throw buildNotFoundError(old_string, finalOldString, content); + if ( + expected_replacements !== undefined && + occurrences !== expected_replacements + ) { throw new Error( - `String to replace not found in file.\nString: ${old_string}`, + `Expected ${expected_replacements} occurrence${expected_replacements === 1 ? "" : "s"} but found ${occurrences}. Update old_string to be more specific, or set replace_all/expected_replacements correctly.`, ); + } + const effectiveReplaceAll = + replace_all || + (expected_replacements !== undefined && expected_replacements > 1); let newContent: string; let replacements: number; - if (replace_all) { - newContent = content.split(old_string).join(new_string); + if (effectiveReplaceAll) { + newContent = content.split(finalOldString).join(finalNewString); replacements = occurrences; } else { - const index = content.indexOf(old_string); + const index = content.indexOf(finalOldString); if (index === -1) - throw new Error(`String not found in file: ${old_string}`); + throw new Error(`String not found in file: ${finalOldString}`); newContent = content.substring(0, index) + - new_string + - content.substring(index + old_string.length); + finalNewString + + content.substring(index + finalOldString.length); replacements = 1; } await fs.writeFile(resolvedPath, newContent, "utf-8"); diff --git a/src/tools/impl/ReplaceGemini.ts b/src/tools/impl/ReplaceGemini.ts index 1451bb4..a3f2d75 100644 --- a/src/tools/impl/ReplaceGemini.ts +++ b/src/tools/impl/ReplaceGemini.ts @@ -16,7 +16,7 @@ export async function replace( args: ReplaceGeminiArgs, ): Promise<{ message: string }> { // Adapt Gemini params to Letta Code's Edit tool - // Gemini uses expected_replacements, Letta Code uses replace_all + // expected_replacements acts as a safety check and determines multi-replace behavior. const lettaArgs = { file_path: args.file_path, old_string: args.old_string, @@ -24,6 +24,7 @@ export async function replace( replace_all: !!( args.expected_replacements && args.expected_replacements > 1 ), + expected_replacements: args.expected_replacements, }; const result = await edit(lettaArgs);