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.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..d0fc11d70 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -55,6 +55,76 @@ function replaceOnceLiteral(content: string, oldString: string, newString: strin return content.slice(0, index) + newString + content.slice(index + oldString.length); } +/** + * 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, + variant: 'collapse-only' | 'escape' | 'unescape-seq-first', +): string { + 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 === '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'); + } + // 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('\\\\', '\\'); +} + +function findBackslashAdjustedMatch( + content: string, + oldString: string, +): { adjusted: string; variant: string } | undefined { + // 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-seq-first'), variant: 'unescape-seq-first' }, + ]; + for (const c of candidates) { + if (c.adjusted !== oldString && content.includes(c.adjusted)) { + return c; + } + } + 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; @@ -71,6 +141,18 @@ export class EditTool implements BuiltinTool { 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.', + }; + } + + // 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}`, @@ -93,19 +175,30 @@ export class EditTool implements BuiltinTool { } 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; + // 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) { + 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.`, + }; + } + } + if (!replaceAll) { let count = 0; let pos = 0; @@ -117,7 +210,8 @@ export class EditTool implements BuiltinTool { } 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, 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. ` }; } if (count > 1) { @@ -140,7 +234,8 @@ export class EditTool implements BuiltinTool { const parts = content.split(args.old_string); 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, 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. ` }; } diff --git a/packages/agent-core/test/tools/edit.test.ts b/packages/agent-core/test/tools/edit.test.ts index 6e0ad2010..b010a18a9 100644 --- a/packages/agent-core/test/tools/edit.test.ts +++ b/packages/agent-core/test/tools/edit.test.ts @@ -433,4 +433,82 @@ 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('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";'; + const tool = new EditTool( + createFakeKaos({ + readText: vi.fn().mockResolvedValue(fileContent), + writeText, + }), + PERMISSIVE_WORKSPACE, + ); + + // LLM sent real newlines (\n as one char) instead of backslash + n (two chars). + // The tool should return a diagnostic error instead of silently adjusting. + const result = await executeTool(tool, + context({ + path: '/tmp/config.txt', + old_string: 'path = "C:\nUsers\nadmin";', + new_string: '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 () => { + 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(); + }); });