From f9a42170c3cb2a7ace4e0819ac4e5d0c0144e098 Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 17:06:39 +0100 Subject: [PATCH 01/10] fix(agent-core): improve Edit tool backslash handling and error diagnostics When old_string is not found, the Edit tool now attempts a backslash unescape fallback to recover from common LLM JSON encoding mistakes (e.g. double-escaped backslashes). Fixes #601. Changes: - Added findBackslashAdjustedMatch() that tries common backslash encoding fixes when old_string doesn't match - Added buildNotFoundHint() that shows the closest matching line in the file when old_string is not found - Updated edit.md with guidance on backslash encoding Tests: - Matches files with multiple literal backslashes (Rust string escapes) - Recovers when old_string has double-escaped backslashes from LLM - Provides diagnostic hint when old_string with backslashes not found --- .../agent-core/src/tools/builtin/file/edit.md | 1 + .../agent-core/src/tools/builtin/file/edit.ts | 97 +++++++++++++++++-- packages/agent-core/test/tools/edit.test.ts | 79 +++++++++++++++ 3 files changed, 169 insertions(+), 8 deletions(-) diff --git a/packages/agent-core/src/tools/builtin/file/edit.md b/packages/agent-core/src/tools/builtin/file/edit.md index 3123f33fa..c81ee752b 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.md +++ b/packages/agent-core/src/tools/builtin/file/edit.md @@ -11,3 +11,4 @@ Perform exact replacements in existing files. - A write lock serializes same-file edits in response order, but serialization does not make stale `old_string` valid. - For pure CRLF files, Read shows LF; use LF in `old_string` and `new_string`, and Edit writes CRLF back. - For mixed endings or lone carriage returns, Read shows carriage returns as \r; include actual \r escapes in those positions. +- Backslash encoding: when old_string contains backslashes (e.g. Rust `\n`, JSON `\\`, regex `\d`), copy the exact characters from the Read output. Do NOT add extra escaping — the file contains literal backslash characters, not escape sequences. diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index a5c77a2a4..6fac11b97 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -55,6 +55,73 @@ function replaceOnceLiteral(content: string, oldString: string, newString: strin return content.slice(0, index) + newString + content.slice(index + oldString.length); } +/** + * Attempt to fix common LLM backslash-encoding mistakes in `old_string`. + * + * Models sometimes double-escape backslashes when encoding old_string in + * JSON (e.g. sending `\\\\n` when the file contains `\\n`). This helper + * tries progressively less-escaped variants and returns the first one + * that matches in `content`, or `undefined` if none match. + */ +function findBackslashAdjustedMatch( + content: string, + oldString: string, +): { adjusted: string; variant: string } | undefined { + const variants: Array<{ adjusted: string; variant: string }> = []; + + // Variant 1: the LLM double-escaped — unescape one level. + // "\\\\n" in JS string → "\\n" (backslash + n), but the file has "\n" (backslash + n). + // If old_string contains sequences like "\\n" that the file doesn't have, + // try replacing "\\n" → "\n" etc. + const unescaped = oldString + .replace(/\\\\n/g, '\n') + .replace(/\\\\t/g, '\t') + .replace(/\\\\r/g, '\r') + .replace(/\\\\\\\\/g, '\\'); + if (unescaped !== oldString) { + variants.push({ adjusted: unescaped, variant: 'unescape-sequences' }); + } + + // Variant 2: the LLM under-escaped — the file has literal double-backslash + // but old_string has single. Try the reverse. + const overEscaped = oldString + .replace(/\\n/g, '\\n') + .replace(/\\t/g, '\\t') + .replace(/\\r/g, '\\r'); + // Only try this if it actually changed something and differs from variant 1. + if (overEscaped !== oldString && overEscaped !== unescaped) { + variants.push({ adjusted: overEscaped, variant: 'escape-sequences' }); + } + + for (const v of variants) { + if (content.indexOf(v.adjusted) !== -1) { + return v; + } + } + return undefined; +} + +/** + * Build a short diagnostic snippet showing the first divergence between + * `oldString` and the file content, helpful when old_string is not found. + */ +function buildNotFoundHint(content: string, oldString: string, filePath: string): string { + const lines = content.split('\n'); + const firstLine = oldString.split('\n')[0] ?? ''; + if (firstLine.length === 0) return ''; + + // Find the closest matching line in the file content. + const candidates = lines.filter((l) => l.includes(firstLine.slice(0, 20))); + if (candidates.length === 0) { + return `The file does not contain a line matching the start of old_string: "${truncate(firstLine, 60)}"`; + } + return `Closest match in file: "${truncate(candidates[0]!, 80)}"`; +} + +function truncate(s: string, max: number): string { + return s.length <= max ? s : s.slice(0, max - 3) + '...'; +} + export class EditTool implements BuiltinTool { readonly name = 'Edit' as const; readonly description = EDIT_DESCRIPTION; @@ -106,18 +173,31 @@ export class EditTool implements BuiltinTool { const content = modelView.text; const replaceAll = args.replace_all ?? false; + // Try the original old_string first; if not found, attempt backslash + // encoding fixes that cover common LLM mis-encodings. + let oldString = args.old_string; + let backslashHint: string | undefined; + if (content.indexOf(oldString) === -1) { + const adjusted = findBackslashAdjustedMatch(content, oldString); + if (adjusted !== undefined) { + oldString = adjusted.adjusted; + backslashHint = ` (matched after ${adjusted.variant} adjustment)`; + } + } + if (!replaceAll) { let count = 0; let pos = 0; while (pos < content.length) { - const idx = content.indexOf(args.old_string, pos); + const idx = content.indexOf(oldString, pos); if (idx === -1) break; count++; - pos = idx + args.old_string.length; + pos = idx + oldString.length; } if (count === 0) { - return { isError: true, output: `old_string not found in ${args.path}, the file contents may be out of date. Please use the Read Tool to reload the content. + const hint = buildNotFoundHint(content, oldString, args.path); + return { isError: true, output: `old_string not found in ${args.path}. ${hint}\nThe file contents may be out of date. Please use the Read Tool to reload the content. ` }; } if (count > 1) { @@ -129,18 +209,19 @@ export class EditTool implements BuiltinTool { }; } - const newContent = replaceOnceLiteral(content, args.old_string, args.new_string); + const newContent = replaceOnceLiteral(content, oldString, args.new_string); await this.kaos.writeText( safePath, materializeModelText(newContent, modelView.lineEndingStyle), ); - return { output: `Replaced 1 occurrence in ${args.path}` }; + return { output: `Replaced 1 occurrence in ${args.path}${backslashHint ?? ''}` }; } - const parts = content.split(args.old_string); + const parts = content.split(oldString); const replacementCount = parts.length - 1; if (replacementCount === 0) { - return { isError: true, output: `old_string not found in ${args.path}, the file contents may be out of date. Please use the Read Tool to reload the content. + const hint = buildNotFoundHint(content, oldString, args.path); + return { isError: true, output: `old_string not found in ${args.path}. ${hint}\nThe file contents may be out of date. Please use the Read Tool to reload the content. ` }; } @@ -149,7 +230,7 @@ export class EditTool implements BuiltinTool { safePath, materializeModelText(newContent, modelView.lineEndingStyle), ); - return { output: `Replaced ${String(replacementCount)} occurrences in ${args.path}` }; + return { output: `Replaced ${String(replacementCount)} occurrences in ${args.path}${backslashHint ?? ''}` }; } catch (error) { const code = (error as { code?: unknown } | null)?.code; if (code === 'EISDIR') { diff --git a/packages/agent-core/test/tools/edit.test.ts b/packages/agent-core/test/tools/edit.test.ts index 6e0ad2010..0c817fe80 100644 --- a/packages/agent-core/test/tools/edit.test.ts +++ b/packages/agent-core/test/tools/edit.test.ts @@ -433,4 +433,83 @@ describe('EditTool', () => { expect(result.isError).toBeFalsy(); expect(writeText).toHaveBeenCalledWith('/workspace-sneaky/test.txt', 'new'); }); + + it('matches files with multiple literal backslashes (Rust string escapes)', async () => { + const writeText = vi.fn().mockResolvedValue(0); + const fileContent = 'let s = "hello\\nworld";\nlet t = "foo\\tbar";'; + const tool = new EditTool( + createFakeKaos({ + readText: vi.fn().mockResolvedValue(fileContent), + writeText, + }), + PERMISSIVE_WORKSPACE, + ); + + const result = await executeTool(tool, + context({ + path: '/tmp/rust.rs', + old_string: 'let s = "hello\\nworld";', + new_string: 'let s = "hello\\nuniverse";', + }), + ); + + expect(result.output).toContain('Replaced 1 occurrence'); + expect(writeText).toHaveBeenCalledWith( + '/tmp/rust.rs', + 'let s = "hello\\nuniverse";\nlet t = "foo\\tbar";', + ); + }); + + it('recovers when old_string has double-escaped backslashes from LLM JSON encoding', async () => { + const writeText = vi.fn().mockResolvedValue(0); + // File has a literal backslash followed by 'n' (two chars). + const fileContent = 'path = "C:\\nUsers\\nadmin";'; + const tool = new EditTool( + createFakeKaos({ + readText: vi.fn().mockResolvedValue(fileContent), + writeText, + }), + PERMISSIVE_WORKSPACE, + ); + + // LLM sent double-escaped: in JS string this is "\\n" (backslash + n) + // but it was meant to match the literal "\n" in the file. + const result = await executeTool(tool, + context({ + path: '/tmp/config.txt', + old_string: 'path = "C:\\nUsers\\nadmin";', + new_string: 'path = "D:\\nUsers\\nadmin";', + }), + ); + + expect(result.output).toContain('Replaced 1 occurrence'); + expect(writeText).toHaveBeenCalledWith( + '/tmp/config.txt', + 'path = "D:\\nUsers\\nadmin";', + ); + }); + + it('provides a diagnostic hint when old_string with backslashes is not found', async () => { + const writeText = vi.fn().mockResolvedValue(0); + const tool = new EditTool( + createFakeKaos({ + readText: vi.fn().mockResolvedValue('alpha beta gamma'), + writeText, + }), + PERMISSIVE_WORKSPACE, + ); + + const result = await executeTool(tool, + context({ + path: '/tmp/a.txt', + old_string: 'delta\\nepsilon', + new_string: 'new', + }), + ); + + expect(result).toMatchObject({ isError: true }); + expect(result.output).toContain('old_string not found'); + expect(result.output).toContain('The file does not contain a line matching'); + expect(writeText).not.toHaveBeenCalled(); + }); }); From 16ba388a21a25c15021dee661185c11f3a6704ab Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 19:38:06 +0100 Subject: [PATCH 02/10] fix: address lint warnings and add changeset for Edit tool backslash fix - Use replaceAll with string args instead of regex for simple patterns - Use includes() instead of indexOf() !== -1 - Prefix unused filePath parameter with _ --- .changeset/fix-edit-backslash-handling.md | 7 +++++++ .../agent-core/src/tools/builtin/file/edit.ts | 20 +++++++++---------- 2 files changed, 17 insertions(+), 10 deletions(-) create mode 100644 .changeset/fix-edit-backslash-handling.md diff --git a/.changeset/fix-edit-backslash-handling.md b/.changeset/fix-edit-backslash-handling.md new file mode 100644 index 000000000..dddf1951c --- /dev/null +++ b/.changeset/fix-edit-backslash-handling.md @@ -0,0 +1,7 @@ +--- +"@moonshot-ai/agent-core": patch +"@moonshot-ai/kimi-code": patch + +--- + +Improve Edit tool recovery when old_string contains misencoded backslashes from the LLM. diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index 6fac11b97..3c3f4481d 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -74,10 +74,10 @@ function findBackslashAdjustedMatch( // If old_string contains sequences like "\\n" that the file doesn't have, // try replacing "\\n" → "\n" etc. const unescaped = oldString - .replace(/\\\\n/g, '\n') - .replace(/\\\\t/g, '\t') - .replace(/\\\\r/g, '\r') - .replace(/\\\\\\\\/g, '\\'); + .replaceAll('\\n', '\n') + .replaceAll('\\t', '\t') + .replaceAll('\\r', '\r') + .replaceAll('\\\\', '\\'); if (unescaped !== oldString) { variants.push({ adjusted: unescaped, variant: 'unescape-sequences' }); } @@ -85,16 +85,16 @@ function findBackslashAdjustedMatch( // Variant 2: the LLM under-escaped — the file has literal double-backslash // but old_string has single. Try the reverse. const overEscaped = oldString - .replace(/\\n/g, '\\n') - .replace(/\\t/g, '\\t') - .replace(/\\r/g, '\\r'); + .replaceAll(/\\n/g, '\\n') + .replaceAll(/\\t/g, '\\t') + .replaceAll(/\\r/g, '\\r'); // Only try this if it actually changed something and differs from variant 1. if (overEscaped !== oldString && overEscaped !== unescaped) { variants.push({ adjusted: overEscaped, variant: 'escape-sequences' }); } for (const v of variants) { - if (content.indexOf(v.adjusted) !== -1) { + if (content.includes(v.adjusted)) { return v; } } @@ -105,7 +105,7 @@ function findBackslashAdjustedMatch( * Build a short diagnostic snippet showing the first divergence between * `oldString` and the file content, helpful when old_string is not found. */ -function buildNotFoundHint(content: string, oldString: string, filePath: string): string { +function buildNotFoundHint(content: string, oldString: string, _filePath: string): string { const lines = content.split('\n'); const firstLine = oldString.split('\n')[0] ?? ''; if (firstLine.length === 0) return ''; @@ -177,7 +177,7 @@ export class EditTool implements BuiltinTool { // encoding fixes that cover common LLM mis-encodings. let oldString = args.old_string; let backslashHint: string | undefined; - if (content.indexOf(oldString) === -1) { + if (!content.includes(oldString)) { const adjusted = findBackslashAdjustedMatch(content, oldString); if (adjusted !== undefined) { oldString = adjusted.adjusted; From 1637617b7b9960c0ee5f75c3a4645553772fa17c Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 22:08:47 +0100 Subject: [PATCH 03/10] fix(agent-core): correct replacement order in backslash unescape fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapse doubled backslashes (\\\\ → \\) BEFORE translating escape sequences (\\n → newline). The previous order converted the second backslash of \\\\n into a real newline before the doubled backslash could be collapsed, producing an incorrect candidate that never matched. Also fixed the overEscaped variant to use replaceAll with string args instead of regex, and correctly re-escape escape sequences. --- .../agent-core/src/tools/builtin/file/edit.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index 3c3f4481d..d66eec574 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -70,24 +70,24 @@ function findBackslashAdjustedMatch( const variants: Array<{ adjusted: string; variant: string }> = []; // Variant 1: the LLM double-escaped — unescape one level. - // "\\\\n" in JS string → "\\n" (backslash + n), but the file has "\n" (backslash + n). - // If old_string contains sequences like "\\n" that the file doesn't have, - // try replacing "\\n" → "\n" etc. + // Collapse doubled backslashes first, then translate escape sequences. + // Order matters: "\\\\" must collapse before "\\n" is processed, + // otherwise the second backslash pairs with 'n' into a real newline. const unescaped = oldString + .replaceAll('\\\\', '\\') .replaceAll('\\n', '\n') .replaceAll('\\t', '\t') - .replaceAll('\\r', '\r') - .replaceAll('\\\\', '\\'); + .replaceAll('\\r', '\r'); if (unescaped !== oldString) { variants.push({ adjusted: unescaped, variant: 'unescape-sequences' }); } // Variant 2: the LLM under-escaped — the file has literal double-backslash - // but old_string has single. Try the reverse. + // but old_string has single. Re-escape common escape sequences. const overEscaped = oldString - .replaceAll(/\\n/g, '\\n') - .replaceAll(/\\t/g, '\\t') - .replaceAll(/\\r/g, '\\r'); + .replaceAll('\\n', '\\\\n') + .replaceAll('\\t', '\\\\t') + .replaceAll('\\r', '\\\\r'); // Only try this if it actually changed something and differs from variant 1. if (overEscaped !== oldString && overEscaped !== unescaped) { variants.push({ adjusted: overEscaped, variant: 'escape-sequences' }); From 68a203ce46fc7a03dd44c02adc5bb5cfb7bac4b0 Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 22:26:24 +0100 Subject: [PATCH 04/10] fix(agent-core): apply backslash fix to both old_string and new_string When the backslash encoding fallback adjusts old_string, new_string must receive the same transformation. Otherwise the replacement text has mismatched encoding (e.g. old_string gets backslash-n but new_string keeps real newlines), producing incorrect output. Also refactored the fix logic into a shared applyBackslashFix() helper used by both variants, and fixed the overEscaped variant which was a no-op (replaced backslash-n with backslash-n instead of replacing real newlines with backslash-n). --- .../agent-core/src/tools/builtin/file/edit.ts | 59 ++++++++++--------- packages/agent-core/test/tools/edit.test.ts | 13 ++-- 2 files changed, 37 insertions(+), 35 deletions(-) diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index d66eec574..9649bae08 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -56,41 +56,40 @@ function replaceOnceLiteral(content: string, oldString: string, newString: strin } /** - * Attempt to fix common LLM backslash-encoding mistakes in `old_string`. - * - * Models sometimes double-escape backslashes when encoding old_string in - * JSON (e.g. sending `\\\\n` when the file contains `\\n`). This helper - * tries progressively less-escaped variants and returns the first one - * that matches in `content`, or `undefined` if none match. + * Attempt to fix common LLM backslash-encoding mistakes in a string. + * Used for both old_string (matched against content) and new_string + * (applied as replacement). */ +function applyBackslashFix( + input: string, + variant: 'unescape' | 'escape', +): string { + if (variant === 'unescape') { + return input + .replaceAll('\\n', '\n') + .replaceAll('\\t', '\t') + .replaceAll('\\r', '\r') + .replaceAll('\\\\', '\\'); + } + return input + .replaceAll('\n', '\\n') + .replaceAll('\t', '\\t') + .replaceAll('\r', '\\r'); +} function findBackslashAdjustedMatch( content: string, oldString: string, -): { adjusted: string; variant: string } | undefined { - const variants: Array<{ adjusted: string; variant: string }> = []; - - // Variant 1: the LLM double-escaped — unescape one level. - // Collapse doubled backslashes first, then translate escape sequences. - // Order matters: "\\\\" must collapse before "\\n" is processed, - // otherwise the second backslash pairs with 'n' into a real newline. - const unescaped = oldString - .replaceAll('\\\\', '\\') - .replaceAll('\\n', '\n') - .replaceAll('\\t', '\t') - .replaceAll('\\r', '\r'); +): { adjusted: string; variant: 'unescape' | 'escape' } | undefined { + const variants: Array<{ adjusted: string; variant: 'unescape' | 'escape' }> = []; + + const unescaped = applyBackslashFix(oldString, 'unescape'); if (unescaped !== oldString) { - variants.push({ adjusted: unescaped, variant: 'unescape-sequences' }); + variants.push({ adjusted: unescaped, variant: 'unescape' }); } - // Variant 2: the LLM under-escaped — the file has literal double-backslash - // but old_string has single. Re-escape common escape sequences. - const overEscaped = oldString - .replaceAll('\\n', '\\\\n') - .replaceAll('\\t', '\\\\t') - .replaceAll('\\r', '\\\\r'); - // Only try this if it actually changed something and differs from variant 1. + const overEscaped = applyBackslashFix(oldString, 'escape'); if (overEscaped !== oldString && overEscaped !== unescaped) { - variants.push({ adjusted: overEscaped, variant: 'escape-sequences' }); + variants.push({ adjusted: overEscaped, variant: 'escape' }); } for (const v of variants) { @@ -176,11 +175,13 @@ export class EditTool implements BuiltinTool { // Try the original old_string first; if not found, attempt backslash // encoding fixes that cover common LLM mis-encodings. let oldString = args.old_string; + let newString = args.new_string; let backslashHint: string | undefined; if (!content.includes(oldString)) { const adjusted = findBackslashAdjustedMatch(content, oldString); if (adjusted !== undefined) { oldString = adjusted.adjusted; + newString = applyBackslashFix(newString, adjusted.variant); backslashHint = ` (matched after ${adjusted.variant} adjustment)`; } } @@ -209,7 +210,7 @@ export class EditTool implements BuiltinTool { }; } - const newContent = replaceOnceLiteral(content, oldString, args.new_string); + const newContent = replaceOnceLiteral(content, oldString, newString); await this.kaos.writeText( safePath, materializeModelText(newContent, modelView.lineEndingStyle), @@ -225,7 +226,7 @@ export class EditTool implements BuiltinTool { ` }; } - const newContent = parts.join(args.new_string); + const newContent = parts.join(newString); await this.kaos.writeText( safePath, materializeModelText(newContent, modelView.lineEndingStyle), diff --git a/packages/agent-core/test/tools/edit.test.ts b/packages/agent-core/test/tools/edit.test.ts index 0c817fe80..0d22d3b63 100644 --- a/packages/agent-core/test/tools/edit.test.ts +++ b/packages/agent-core/test/tools/edit.test.ts @@ -460,9 +460,9 @@ describe('EditTool', () => { ); }); - it('recovers when old_string has double-escaped backslashes from LLM JSON encoding', async () => { + it('recovers when old_string has real newlines but file has literal backslash-n', async () => { const writeText = vi.fn().mockResolvedValue(0); - // File has a literal backslash followed by 'n' (two chars). + // File has backslash + n (two chars): the literal string \n const fileContent = 'path = "C:\\nUsers\\nadmin";'; const tool = new EditTool( createFakeKaos({ @@ -472,17 +472,18 @@ describe('EditTool', () => { PERMISSIVE_WORKSPACE, ); - // LLM sent double-escaped: in JS string this is "\\n" (backslash + n) - // but it was meant to match the literal "\n" in the file. + // LLM sent real newlines (\n as one char) instead of backslash + n (two chars). + // The unescape fallback should convert \n → backslash + n to match the file. const result = await executeTool(tool, context({ path: '/tmp/config.txt', - old_string: 'path = "C:\\nUsers\\nadmin";', - new_string: 'path = "D:\\nUsers\\nadmin";', + old_string: 'path = "C:\nUsers\nadmin";', + new_string: 'path = "D:\nUsers\nadmin";', }), ); expect(result.output).toContain('Replaced 1 occurrence'); + expect(result.output).toContain('escape adjustment'); expect(writeText).toHaveBeenCalledWith( '/tmp/config.txt', 'path = "D:\\nUsers\\nadmin";', From 0fb69c44de717d83ef50718b3812683d605bc4e7 Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 22:39:05 +0100 Subject: [PATCH 05/10] fix(agent-core): check escape variant before unescape in backslash fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The escape variant (real newlines → backslash-n) must be checked before the unescape variant (doubled backslashes → single). Otherwise the unescape variant incorrectly matches real newlines, producing backslash + newline instead of the intended backslash + n. --- .../agent-core/src/tools/builtin/file/edit.ts | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index 9649bae08..30c70440f 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -80,23 +80,19 @@ function findBackslashAdjustedMatch( content: string, oldString: string, ): { adjusted: string; variant: 'unescape' | 'escape' } | undefined { - const variants: Array<{ adjusted: string; variant: 'unescape' | 'escape' }> = []; - - const unescaped = applyBackslashFix(oldString, 'unescape'); - if (unescaped !== oldString) { - variants.push({ adjusted: unescaped, variant: 'unescape' }); - } - + // Check escape first (handles real newlines → backslash-n), then unescape + // (handles doubled backslashes → single). Order matters: escape must come + // first because unescape would incorrectly match real newlines. const overEscaped = applyBackslashFix(oldString, 'escape'); - if (overEscaped !== oldString && overEscaped !== unescaped) { - variants.push({ adjusted: overEscaped, variant: 'escape' }); + if (overEscaped !== oldString && content.includes(overEscaped)) { + return { adjusted: overEscaped, variant: 'escape' }; } - for (const v of variants) { - if (content.includes(v.adjusted)) { - return v; - } + const unescaped = applyBackslashFix(oldString, 'unescape'); + if (unescaped !== oldString && content.includes(unescaped)) { + return { adjusted: unescaped, variant: 'unescape' }; } + return undefined; } From 31705a2512946a90d4c2e85fae5420f20b786537 Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Thu, 11 Jun 2026 22:46:01 +0100 Subject: [PATCH 06/10] fix(agent-core): handle all three backslash encoding variants separately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No single replacement order handles all LLM encoding mistakes. Split into three variants: - collapse-first: \\\\n → \\n (double backslash collapsed before escape seq) - seq-first: \\n → \\n (real newline converted before collapse) - escape: \\n → \\\\n (real newline re-escaped to backslash-n) Each handles a distinct encoding mistake. The first matching variant is used for both old_string and new_string. --- .../agent-core/src/tools/builtin/file/edit.ts | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index 30c70440f..32d90b681 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -62,37 +62,53 @@ function replaceOnceLiteral(content: string, oldString: string, newString: strin */ function applyBackslashFix( input: string, - variant: 'unescape' | 'escape', + variant: 'unescape-collapse-first' | 'unescape-seq-first' | 'escape', ): string { - if (variant === 'unescape') { + if (variant === 'unescape-collapse-first') { + // Collapse doubled backslashes first, then translate escape sequences. + // Handles: LLM sent \\n (two backslashes + n) but file has \n (one backslash + n). + return input + .replaceAll('\\\\', '\\') + .replaceAll('\\n', '\n') + .replaceAll('\\t', '\t') + .replaceAll('\\r', '\r'); + } + if (variant === 'unescape-seq-first') { + // Translate escape sequences first, then collapse doubled backslashes. + // Handles: LLM sent \n (real newline) but file has \n (backslash + n). return input .replaceAll('\\n', '\n') .replaceAll('\\t', '\t') .replaceAll('\\r', '\r') .replaceAll('\\\\', '\\'); } + // escape: replace real newlines with backslash-n. + // Handles: LLM sent \n (real newline) but file has \\n (two backslashes + n). return input .replaceAll('\n', '\\n') .replaceAll('\t', '\\t') .replaceAll('\r', '\\r'); } + function findBackslashAdjustedMatch( content: string, oldString: string, -): { adjusted: string; variant: 'unescape' | 'escape' } | undefined { - // Check escape first (handles real newlines → backslash-n), then unescape - // (handles doubled backslashes → single). Order matters: escape must come - // first because unescape would incorrectly match real newlines. - const overEscaped = applyBackslashFix(oldString, 'escape'); - if (overEscaped !== oldString && content.includes(overEscaped)) { - return { adjusted: overEscaped, variant: 'escape' }; - } - - const unescaped = applyBackslashFix(oldString, 'unescape'); - if (unescaped !== oldString && content.includes(unescaped)) { - return { adjusted: unescaped, variant: 'unescape' }; +): { adjusted: string; variant: string } | undefined { + // Try all three variants and return the first that matches the file content. + // Each handles a different LLM encoding mistake: + // collapse-first: \\n → \n (double backslash collapsed before escape seq) + // seq-first: \n → \n (real newline converted before collapse) + // escape: \n → \\n (real newline re-escaped to backslash-n) + const candidates: Array<{ adjusted: string; variant: string }> = [ + { adjusted: applyBackslashFix(oldString, 'escape'), variant: 'escape' }, + { adjusted: applyBackslashFix(oldString, 'unescape-collapse-first'), variant: 'unescape-collapse-first' }, + { adjusted: applyBackslashFix(oldString, 'unescape-seq-first'), variant: 'unescape-seq-first' }, + ]; + for (const c of candidates) { + if (c.adjusted !== oldString && content.includes(c.adjusted)) { + return c; + } } - return undefined; } @@ -177,7 +193,7 @@ export class EditTool implements BuiltinTool { const adjusted = findBackslashAdjustedMatch(content, oldString); if (adjusted !== undefined) { oldString = adjusted.adjusted; - newString = applyBackslashFix(newString, adjusted.variant); + newString = applyBackslashFix(newString, adjusted.variant as 'escape' | 'unescape-collapse-first' | 'unescape-seq-first'); backslashHint = ` (matched after ${adjusted.variant} adjustment)`; } } From 27a38928cd5bf443a214304d41e0b016081c2770 Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Fri, 12 Jun 2026 04:55:28 +0100 Subject: [PATCH 07/10] fix(agent-core): add collapse-only variant for double-backslash case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The collapse-first variant still processed escape sequences after collapsing, breaking the double-backslash case. Added a pure collapse-only variant (\\\\ → \\) that handles LLM double-escaping without touching escape sequences. Also cleaned up orphaned duplicate function definitions. --- .../agent-core/src/tools/builtin/file/edit.ts | 48 ++++++++----------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index 32d90b681..c9d689cec 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -62,46 +62,38 @@ function replaceOnceLiteral(content: string, oldString: string, newString: strin */ function applyBackslashFix( input: string, - variant: 'unescape-collapse-first' | 'unescape-seq-first' | 'escape', + variant: 'collapse-only' | 'escape' | 'unescape-seq-first', ): string { - if (variant === 'unescape-collapse-first') { - // Collapse doubled backslashes first, then translate escape sequences. - // Handles: LLM sent \\n (two backslashes + n) but file has \n (one backslash + n). - return input - .replaceAll('\\\\', '\\') - .replaceAll('\\n', '\n') - .replaceAll('\\t', '\t') - .replaceAll('\\r', '\r'); + if (variant === 'collapse-only') { + // Only collapse doubled backslashes — no escape sequence processing. + // Handles: LLM sent \\\\n (two backslashes + n) but file has \n (one backslash + n). + return input.replaceAll('\\\\', '\\'); } - if (variant === 'unescape-seq-first') { - // Translate escape sequences first, then collapse doubled backslashes. - // Handles: LLM sent \n (real newline) but file has \n (backslash + n). + if (variant === 'escape') { + // Replace real newlines with backslash-n. + // Handles: LLM sent \n (real newline) but file has \\n (two backslashes + n). return input - .replaceAll('\\n', '\n') - .replaceAll('\\t', '\t') - .replaceAll('\\r', '\r') - .replaceAll('\\\\', '\\'); + .replaceAll('\n', '\\n') + .replaceAll('\t', '\\t') + .replaceAll('\r', '\\r'); } - // escape: replace real newlines with backslash-n. - // Handles: LLM sent \n (real newline) but file has \\n (two backslashes + n). + // unescape-seq-first: translate escape sequences first, then collapse. + // Handles: LLM sent \n (real newline) but file has \n (backslash + n). return input - .replaceAll('\n', '\\n') - .replaceAll('\t', '\\t') - .replaceAll('\r', '\\r'); + .replaceAll('\\n', '\n') + .replaceAll('\\t', '\t') + .replaceAll('\\r', '\r') + .replaceAll('\\\\', '\\'); } function findBackslashAdjustedMatch( content: string, oldString: string, ): { adjusted: string; variant: string } | undefined { - // Try all three variants and return the first that matches the file content. - // Each handles a different LLM encoding mistake: - // collapse-first: \\n → \n (double backslash collapsed before escape seq) - // seq-first: \n → \n (real newline converted before collapse) - // escape: \n → \\n (real newline re-escaped to backslash-n) + // Try all variants and return the first that matches the file content. const candidates: Array<{ adjusted: string; variant: string }> = [ + { adjusted: applyBackslashFix(oldString, 'collapse-only'), variant: 'collapse-only' }, { adjusted: applyBackslashFix(oldString, 'escape'), variant: 'escape' }, - { adjusted: applyBackslashFix(oldString, 'unescape-collapse-first'), variant: 'unescape-collapse-first' }, { adjusted: applyBackslashFix(oldString, 'unescape-seq-first'), variant: 'unescape-seq-first' }, ]; for (const c of candidates) { @@ -193,7 +185,7 @@ export class EditTool implements BuiltinTool { const adjusted = findBackslashAdjustedMatch(content, oldString); if (adjusted !== undefined) { oldString = adjusted.adjusted; - newString = applyBackslashFix(newString, adjusted.variant as 'escape' | 'unescape-collapse-first' | 'unescape-seq-first'); + newString = applyBackslashFix(newString, adjusted.variant as 'escape' | 'collapse-only' | 'unescape-seq-first'); backslashHint = ` (matched after ${adjusted.variant} adjustment)`; } } From f524bf0075b3e0175b34f439f561b8d4fe3c881a Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Fri, 12 Jun 2026 05:13:00 +0100 Subject: [PATCH 08/10] fix(agent-core): move backslash fallback to resolveExecution for correct approval display The backslash encoding fallback was applied inside execution() after the approval panel had already rendered with the original args. This meant users approved one diff but the tool applied a different one. Moved the file read and fallback into resolveExecution() so the approval display shows the exact text that will be written. The no-op check was also moved before file I/O to avoid unnecessary reads. --- .../agent-core/src/tools/builtin/file/edit.ts | 79 +++++++++++-------- packages/agent-core/test/tools/edit.test.ts | 5 +- 2 files changed, 47 insertions(+), 37 deletions(-) diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index c9d689cec..1119cb486 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -135,12 +135,44 @@ export class EditTool implements BuiltinTool { private readonly workspace: WorkspaceConfig, ) {} - resolveExecution(args: EditInput): ToolExecution { + async resolveExecution(args: EditInput): Promise { const path = resolvePathAccessPath(args.path, { kaos: this.kaos, workspace: this.workspace, operation: 'write', }); + + // Reject no-op edits before any file I/O. + if (args.old_string === args.new_string) { + return { + isError: true, + output: 'No changes to make: old_string and new_string are exactly the same.', + }; + } + + // Read the file and apply backslash fallback early, so the approval + // display shows the exact text that will be written. + let adjustedArgs = args; + try { + const raw = await this.kaos.readText(path); + const content = toModelTextView(raw).text; + if (!content.includes(args.old_string)) { + const adjusted = findBackslashAdjustedMatch(content, args.old_string); + if (adjusted !== undefined) { + adjustedArgs = { + ...args, + old_string: adjusted.adjusted, + new_string: applyBackslashFix( + args.new_string, + adjusted.variant as 'escape' | 'collapse-only' | 'unescape-seq-first', + ), + }; + } + } + } catch { + // If we can't read the file here, execution() will handle it. + } + return { accesses: ToolAccesses.readWriteFile(path), description: `Editing ${args.path}`, @@ -148,8 +180,8 @@ export class EditTool implements BuiltinTool { kind: 'file_io', operation: 'edit', path, - before: args.old_string, - after: args.new_string, + before: adjustedArgs.old_string, + after: adjustedArgs.new_string, }, approvalRule: literalRulePattern(this.name, path), matchesRule: (ruleArgs) => @@ -158,50 +190,29 @@ export class EditTool implements BuiltinTool { pathClass: this.kaos.pathClass(), homeDir: this.kaos.gethome(), }), - execute: () => this.execution(args, path), + execute: () => this.execution(adjustedArgs, path), }; } private async execution(args: EditInput, safePath: string): Promise { - if (args.old_string === args.new_string) { - return { - isError: true, - output: 'No changes to make: old_string and new_string are exactly the same.', - }; - } - try { const raw = await this.kaos.readText(safePath); const modelView = toModelTextView(raw); const content = modelView.text; const replaceAll = args.replace_all ?? false; - // Try the original old_string first; if not found, attempt backslash - // encoding fixes that cover common LLM mis-encodings. - let oldString = args.old_string; - let newString = args.new_string; - let backslashHint: string | undefined; - if (!content.includes(oldString)) { - const adjusted = findBackslashAdjustedMatch(content, oldString); - if (adjusted !== undefined) { - oldString = adjusted.adjusted; - newString = applyBackslashFix(newString, adjusted.variant as 'escape' | 'collapse-only' | 'unescape-seq-first'); - backslashHint = ` (matched after ${adjusted.variant} adjustment)`; - } - } - if (!replaceAll) { let count = 0; let pos = 0; while (pos < content.length) { - const idx = content.indexOf(oldString, pos); + const idx = content.indexOf(args.old_string, pos); if (idx === -1) break; count++; - pos = idx + oldString.length; + pos = idx + args.old_string.length; } if (count === 0) { - const hint = buildNotFoundHint(content, oldString, args.path); + const hint = buildNotFoundHint(content, args.old_string, args.path); return { isError: true, output: `old_string not found in ${args.path}. ${hint}\nThe file contents may be out of date. Please use the Read Tool to reload the content. ` }; } @@ -214,28 +225,28 @@ export class EditTool implements BuiltinTool { }; } - const newContent = replaceOnceLiteral(content, oldString, newString); + const newContent = replaceOnceLiteral(content, args.old_string, args.new_string); await this.kaos.writeText( safePath, materializeModelText(newContent, modelView.lineEndingStyle), ); - return { output: `Replaced 1 occurrence in ${args.path}${backslashHint ?? ''}` }; + return { output: `Replaced 1 occurrence in ${args.path}` }; } - const parts = content.split(oldString); + const parts = content.split(args.old_string); const replacementCount = parts.length - 1; if (replacementCount === 0) { - const hint = buildNotFoundHint(content, oldString, args.path); + const hint = buildNotFoundHint(content, args.old_string, args.path); return { isError: true, output: `old_string not found in ${args.path}. ${hint}\nThe file contents may be out of date. Please use the Read Tool to reload the content. ` }; } - const newContent = parts.join(newString); + const newContent = parts.join(args.new_string); await this.kaos.writeText( safePath, materializeModelText(newContent, modelView.lineEndingStyle), ); - return { output: `Replaced ${String(replacementCount)} occurrences in ${args.path}${backslashHint ?? ''}` }; + return { output: `Replaced ${String(replacementCount)} occurrences in ${args.path}` }; } catch (error) { const code = (error as { code?: unknown } | null)?.code; if (code === 'EISDIR') { diff --git a/packages/agent-core/test/tools/edit.test.ts b/packages/agent-core/test/tools/edit.test.ts index 0d22d3b63..5bf0d0aec 100644 --- a/packages/agent-core/test/tools/edit.test.ts +++ b/packages/agent-core/test/tools/edit.test.ts @@ -11,9 +11,9 @@ function context(args: EditInput) { } describe('EditTool', () => { - it('exposes before/after on the file_io display so the approval panel can render a diff', () => { + it('exposes before/after on the file_io display so the approval panel can render a diff', async () => { const tool = new EditTool(createFakeKaos(), PERMISSIVE_WORKSPACE); - const execution = tool.resolveExecution({ + const execution = await tool.resolveExecution({ path: '/tmp/foo.ts', old_string: 'a\nb\nc', new_string: 'a\nB\nc', @@ -483,7 +483,6 @@ describe('EditTool', () => { ); expect(result.output).toContain('Replaced 1 occurrence'); - expect(result.output).toContain('escape adjustment'); expect(writeText).toHaveBeenCalledWith( '/tmp/config.txt', 'path = "D:\\nUsers\\nadmin";', From 8a7ca8750e9a43da22d7fb646ac5d90e78e36685 Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Fri, 12 Jun 2026 05:34:22 +0100 Subject: [PATCH 09/10] fix(agent-core): remove file read from resolveExecution to fix P1 auth bypass resolveExecution() runs BEFORE the authorization hook in tool-call.ts. Reading the file there bypasses permission checks for sensitive paths. - Reverted resolveExecution to synchronous (no file I/O) - Moved backslash fallback to execution() which runs AFTER auth - Fallback adjusts both old_string and new_string consistently --- .../agent-core/src/tools/builtin/file/edit.ts | 63 +++++++++---------- packages/agent-core/test/tools/edit.test.ts | 4 +- 2 files changed, 31 insertions(+), 36 deletions(-) diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index 1119cb486..5f0ce23e6 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -135,7 +135,7 @@ export class EditTool implements BuiltinTool { private readonly workspace: WorkspaceConfig, ) {} - async resolveExecution(args: EditInput): Promise { + resolveExecution(args: EditInput): ToolExecution { const path = resolvePathAccessPath(args.path, { kaos: this.kaos, workspace: this.workspace, @@ -150,29 +150,9 @@ export class EditTool implements BuiltinTool { }; } - // Read the file and apply backslash fallback early, so the approval - // display shows the exact text that will be written. - let adjustedArgs = args; - try { - const raw = await this.kaos.readText(path); - const content = toModelTextView(raw).text; - if (!content.includes(args.old_string)) { - const adjusted = findBackslashAdjustedMatch(content, args.old_string); - if (adjusted !== undefined) { - adjustedArgs = { - ...args, - old_string: adjusted.adjusted, - new_string: applyBackslashFix( - args.new_string, - adjusted.variant as 'escape' | 'collapse-only' | 'unescape-seq-first', - ), - }; - } - } - } catch { - // If we can't read the file here, execution() will handle it. - } - + // No file read here — resolveExecution runs BEFORE the authorization + // hook in tool-call.ts. Reading the file here would bypass permission + // checks. The backslash fallback is applied in execution() after auth. return { accesses: ToolAccesses.readWriteFile(path), description: `Editing ${args.path}`, @@ -180,8 +160,8 @@ export class EditTool implements BuiltinTool { kind: 'file_io', operation: 'edit', path, - before: adjustedArgs.old_string, - after: adjustedArgs.new_string, + before: args.old_string, + after: args.new_string, }, approvalRule: literalRulePattern(this.name, path), matchesRule: (ruleArgs) => @@ -190,7 +170,7 @@ export class EditTool implements BuiltinTool { pathClass: this.kaos.pathClass(), homeDir: this.kaos.gethome(), }), - execute: () => this.execution(adjustedArgs, path), + execute: () => this.execution(args, path), }; } @@ -201,18 +181,33 @@ export class EditTool implements BuiltinTool { const content = modelView.text; const replaceAll = args.replace_all ?? false; + // Apply backslash fallback after auth — adjust both old_string and + // new_string with the same transformation so the replacement is correct. + let oldString = args.old_string; + let newString = args.new_string; + if (!content.includes(oldString)) { + const adjusted = findBackslashAdjustedMatch(content, oldString); + if (adjusted !== undefined) { + oldString = adjusted.adjusted; + newString = applyBackslashFix( + newString, + adjusted.variant as 'escape' | 'collapse-only' | 'unescape-seq-first', + ); + } + } + if (!replaceAll) { let count = 0; let pos = 0; while (pos < content.length) { - const idx = content.indexOf(args.old_string, pos); + const idx = content.indexOf(oldString, pos); if (idx === -1) break; count++; - pos = idx + args.old_string.length; + pos = idx + oldString.length; } if (count === 0) { - const hint = buildNotFoundHint(content, args.old_string, args.path); + const hint = buildNotFoundHint(content, oldString, args.path); return { isError: true, output: `old_string not found in ${args.path}. ${hint}\nThe file contents may be out of date. Please use the Read Tool to reload the content. ` }; } @@ -225,7 +220,7 @@ export class EditTool implements BuiltinTool { }; } - const newContent = replaceOnceLiteral(content, args.old_string, args.new_string); + const newContent = replaceOnceLiteral(content, oldString, newString); await this.kaos.writeText( safePath, materializeModelText(newContent, modelView.lineEndingStyle), @@ -233,15 +228,15 @@ export class EditTool implements BuiltinTool { return { output: `Replaced 1 occurrence in ${args.path}` }; } - const parts = content.split(args.old_string); + const parts = content.split(oldString); const replacementCount = parts.length - 1; if (replacementCount === 0) { - const hint = buildNotFoundHint(content, args.old_string, args.path); + const hint = buildNotFoundHint(content, oldString, args.path); return { isError: true, output: `old_string not found in ${args.path}. ${hint}\nThe file contents may be out of date. Please use the Read Tool to reload the content. ` }; } - const newContent = parts.join(args.new_string); + const newContent = parts.join(newString); await this.kaos.writeText( safePath, materializeModelText(newContent, modelView.lineEndingStyle), diff --git a/packages/agent-core/test/tools/edit.test.ts b/packages/agent-core/test/tools/edit.test.ts index 5bf0d0aec..9a6b1a43e 100644 --- a/packages/agent-core/test/tools/edit.test.ts +++ b/packages/agent-core/test/tools/edit.test.ts @@ -11,9 +11,9 @@ function context(args: EditInput) { } describe('EditTool', () => { - it('exposes before/after on the file_io display so the approval panel can render a diff', async () => { + it('exposes before/after on the file_io display so the approval panel can render a diff', () => { const tool = new EditTool(createFakeKaos(), PERMISSIVE_WORKSPACE); - const execution = await tool.resolveExecution({ + const execution = tool.resolveExecution({ path: '/tmp/foo.ts', old_string: 'a\nb\nc', new_string: 'a\nB\nc', From 0d0452f9328611d79ee5db39a9c953107fd68c93 Mon Sep 17 00:00:00 2001 From: LifeJiggy Date: Fri, 12 Jun 2026 05:55:17 +0100 Subject: [PATCH 10/10] fix(agent-core): return diagnostic error instead of silent backslash adjustment The backslash encoding fallback was silently adjusting old_string/new_string after the approval display showed the original text, creating a mismatch between what the user approved and what was applied (P2). Also, resolveExecution runs BEFORE the authorization hook, so reading the file there would bypass permission checks (P1). Fix: return a diagnostic error when a backslash encoding mismatch is detected, telling the LLM to resend with correct encoding. No silent adjustment, no auth bypass, no display mismatch. --- .../agent-core/src/tools/builtin/file/edit.ts | 45 ++++++++++--------- packages/agent-core/test/tools/edit.test.ts | 13 +++--- 2 files changed, 30 insertions(+), 28 deletions(-) diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index 5f0ce23e6..d0fc11d70 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -56,9 +56,9 @@ function replaceOnceLiteral(content: string, oldString: string, newString: strin } /** - * Attempt to fix common LLM backslash-encoding mistakes in a string. - * Used for both old_string (matched against content) and new_string - * (applied as replacement). + * Apply a backslash encoding transformation to detect whether a + * different encoding would match the file content. Used for detection + * only — the actual replacement uses the original args. */ function applyBackslashFix( input: string, @@ -181,18 +181,21 @@ export class EditTool implements BuiltinTool { const content = modelView.text; const replaceAll = args.replace_all ?? false; - // Apply backslash fallback after auth — adjust both old_string and - // new_string with the same transformation so the replacement is correct. - let oldString = args.old_string; - let newString = args.new_string; - if (!content.includes(oldString)) { - const adjusted = findBackslashAdjustedMatch(content, oldString); + // If old_string is not found, check if a backslash encoding mismatch + // is the cause. Instead of silently adjusting (which would mismatch + // the approval display), return a diagnostic error so the LLM can + // resend with the correct encoding. + if (!content.includes(args.old_string)) { + const adjusted = findBackslashAdjustedMatch(content, args.old_string); if (adjusted !== undefined) { - oldString = adjusted.adjusted; - newString = applyBackslashFix( - newString, - adjusted.variant as 'escape' | 'collapse-only' | 'unescape-seq-first', - ); + const hint = buildNotFoundHint(content, args.old_string, args.path); + return { + isError: true, + output: + `old_string not found in ${args.path}. ${hint}\n` + + `The file content differs in backslash encoding (detected: ${adjusted.variant}). ` + + `Use the Read Tool to view the exact file content and resend old_string matching the literal characters shown.`, + }; } } @@ -200,14 +203,14 @@ export class EditTool implements BuiltinTool { let count = 0; let pos = 0; while (pos < content.length) { - const idx = content.indexOf(oldString, pos); + const idx = content.indexOf(args.old_string, pos); if (idx === -1) break; count++; - pos = idx + oldString.length; + pos = idx + args.old_string.length; } if (count === 0) { - const hint = buildNotFoundHint(content, oldString, args.path); + const hint = buildNotFoundHint(content, args.old_string, args.path); return { isError: true, output: `old_string not found in ${args.path}. ${hint}\nThe file contents may be out of date. Please use the Read Tool to reload the content. ` }; } @@ -220,7 +223,7 @@ export class EditTool implements BuiltinTool { }; } - const newContent = replaceOnceLiteral(content, oldString, newString); + const newContent = replaceOnceLiteral(content, args.old_string, args.new_string); await this.kaos.writeText( safePath, materializeModelText(newContent, modelView.lineEndingStyle), @@ -228,15 +231,15 @@ export class EditTool implements BuiltinTool { return { output: `Replaced 1 occurrence in ${args.path}` }; } - const parts = content.split(oldString); + const parts = content.split(args.old_string); const replacementCount = parts.length - 1; if (replacementCount === 0) { - const hint = buildNotFoundHint(content, oldString, args.path); + const hint = buildNotFoundHint(content, args.old_string, args.path); return { isError: true, output: `old_string not found in ${args.path}. ${hint}\nThe file contents may be out of date. Please use the Read Tool to reload the content. ` }; } - const newContent = parts.join(newString); + const newContent = parts.join(args.new_string); await this.kaos.writeText( safePath, materializeModelText(newContent, modelView.lineEndingStyle), diff --git a/packages/agent-core/test/tools/edit.test.ts b/packages/agent-core/test/tools/edit.test.ts index 9a6b1a43e..b010a18a9 100644 --- a/packages/agent-core/test/tools/edit.test.ts +++ b/packages/agent-core/test/tools/edit.test.ts @@ -460,7 +460,7 @@ describe('EditTool', () => { ); }); - it('recovers when old_string has real newlines but file has literal backslash-n', async () => { + it('returns encoding mismatch diagnostic when old_string has real newlines but file has literal backslash-n', async () => { const writeText = vi.fn().mockResolvedValue(0); // File has backslash + n (two chars): the literal string \n const fileContent = 'path = "C:\\nUsers\\nadmin";'; @@ -473,7 +473,7 @@ describe('EditTool', () => { ); // LLM sent real newlines (\n as one char) instead of backslash + n (two chars). - // The unescape fallback should convert \n → backslash + n to match the file. + // The tool should return a diagnostic error instead of silently adjusting. const result = await executeTool(tool, context({ path: '/tmp/config.txt', @@ -482,11 +482,10 @@ describe('EditTool', () => { }), ); - expect(result.output).toContain('Replaced 1 occurrence'); - expect(writeText).toHaveBeenCalledWith( - '/tmp/config.txt', - 'path = "D:\\nUsers\\nadmin";', - ); + expect(result).toMatchObject({ isError: true }); + expect(result.output).toContain('backslash encoding'); + expect(result.output).toContain('Read Tool'); + expect(writeText).not.toHaveBeenCalled(); }); it('provides a diagnostic hint when old_string with backslashes is not found', async () => {