From c3232303549f9dd01cc3c9a73795f0d35c46b96f Mon Sep 17 00:00:00 2001 From: Mangesh Raut Date: Sat, 1 Aug 2026 17:08:00 +0000 Subject: [PATCH 1/2] fix(agent-core): refuse multi-line empty Edit deletions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guard Edit against accidental +0/−N wipeouts when new_string is empty across 3+ lines unless allow_large_delete is set. Strengthen the old_string-not-found recovery guidance so the model rereads a large enough region instead of looping short Reads (fixes #2427). --- .changeset/edit-refuse-large-delete.md | 6 ++ .../src/agent/tools/edit/edit.md | 2 + .../src/agent/tools/edit/edit.ts | 6 ++ .../src/agent/tools/edit/editTool.ts | 1 + .../agent-core-v2/src/app/edit/editService.ts | 37 +++++++++++- .../agent-core-v2/src/app/edit/fileEdit.ts | 1 + .../src/app/edit/fileEditService.ts | 1 + .../test/app/edit/tools/edit.test.ts | 49 ++++++++++++++++ .../agent-core/src/tools/builtin/file/edit.md | 2 + .../agent-core/src/tools/builtin/file/edit.ts | 52 +++++++++++++++-- packages/agent-core/test/tools/edit.test.ts | 58 +++++++++++++++++++ 11 files changed, 209 insertions(+), 6 deletions(-) create mode 100644 .changeset/edit-refuse-large-delete.md diff --git a/.changeset/edit-refuse-large-delete.md b/.changeset/edit-refuse-large-delete.md new file mode 100644 index 00000000000..7b1e31a7181 --- /dev/null +++ b/.changeset/edit-refuse-large-delete.md @@ -0,0 +1,6 @@ +--- +'@moonshot-ai/agent-core': patch +'@moonshot-ai/kimi-code': patch +--- + +Refuse multi-line empty Edit deletions unless `allow_large_delete` is set, and tell the model to reread a large enough region after `old_string not found`. diff --git a/packages/agent-core-v2/src/agent/tools/edit/edit.md b/packages/agent-core-v2/src/agent/tools/edit/edit.md index f928fa22fb6..808e55d826c 100644 --- a/packages/agent-core-v2/src/agent/tools/edit/edit.md +++ b/packages/agent-core-v2/src/agent/tools/edit/edit.md @@ -8,6 +8,8 @@ Perform exact replacements in existing files. - If `old_string` is ambiguous, add surrounding context. Use `replace_all` only when every occurrence should change — for example, renaming a symbol throughout the file. - Multiple Edit calls may run in one response only when they do not target the same file. - DO NOT issue consecutive Edit calls on the same file. A previous Edit can invalidate a later Edit's `old_string`, causing `old_string not found`. Read the file again before the next Edit. +- After Edit fails with `old_string not found`, Read the whole file (or a large enough region covering the edit) once before retrying. Do not loop Edit → short 30–50 line Read → Edit. +- Never replace a multi-line span with an empty `new_string` unless you intend to delete it — set `allow_large_delete=true` for that intentional deletion. Prefer replacing with the real new content. - 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. diff --git a/packages/agent-core-v2/src/agent/tools/edit/edit.ts b/packages/agent-core-v2/src/agent/tools/edit/edit.ts index 8a5d3acce0f..2128ef2a7a3 100644 --- a/packages/agent-core-v2/src/agent/tools/edit/edit.ts +++ b/packages/agent-core-v2/src/agent/tools/edit/edit.ts @@ -24,6 +24,12 @@ export const EditInputSchema = z.object({ .boolean() .optional() .describe('Set true only when every occurrence of old_string should be replaced.'), + allow_large_delete: z + .boolean() + .optional() + .describe( + 'Set true only when intentionally deleting a multi-line span with an empty (or whitespace-only) new_string. Omit for normal edits.', + ), }); export type EditInput = z.infer; diff --git a/packages/agent-core-v2/src/agent/tools/edit/editTool.ts b/packages/agent-core-v2/src/agent/tools/edit/editTool.ts index a3ef1f35cb0..4a85435f494 100644 --- a/packages/agent-core-v2/src/agent/tools/edit/editTool.ts +++ b/packages/agent-core-v2/src/agent/tools/edit/editTool.ts @@ -103,6 +103,7 @@ export class EditTool implements IEditTool { old_string: args.old_string, new_string: args.new_string, replace_all: args.replace_all ?? false, + allow_large_delete: args.allow_large_delete, }, fs); if (!result.ok) { return { isError: true, output: result.error }; diff --git a/packages/agent-core-v2/src/app/edit/editService.ts b/packages/agent-core-v2/src/app/edit/editService.ts index 3c261d4c9c6..8a3640874af 100644 --- a/packages/agent-core-v2/src/app/edit/editService.ts +++ b/packages/agent-core-v2/src/app/edit/editService.ts @@ -5,15 +5,44 @@ export interface EditApplyInput { readonly old_string: string; readonly new_string: string; readonly replace_all: boolean; + readonly allow_large_delete?: boolean; } export type EditApplyResult = | { readonly ok: true; readonly rawContent: string; readonly count: number } | { readonly ok: false; readonly error: string }; +/** Multi-line empty replacements without an explicit opt-in are refused (see #2427). */ +const LARGE_DELETE_MIN_OLD_LINES = 3; + +export function countEditLines(text: string): number { + if (text.length === 0) return 0; + let lines = 1; + for (let i = 0; i < text.length; i++) { + if (text.charCodeAt(i) === 10) lines++; + } + return lines; +} + +export function isOversizedEmptyDeletion(oldString: string, newString: string): boolean { + return newString.trim().length === 0 && countEditLines(oldString) >= LARGE_DELETE_MIN_OLD_LINES; +} + +function oversizedDeletionMessage(path: string): string { + return ( + `Refusing a multi-line deletion in ${path}: new_string is empty (or whitespace-only) while ` + + `old_string spans ${String(LARGE_DELETE_MIN_OLD_LINES)}+ lines. Read the file again, then either ` + + `replace with the intended new content, delete fewer lines at a time, or set allow_large_delete=true ` + + `if you intentionally want to remove that entire span.` + ); +} + function notFoundMessage(path: string): string { - return `old_string not found in ${path}, the file contents may be out of date. Please use the Read Tool to reload the content. -`; + return ( + `old_string not found in ${path}, the file contents may be out of date. ` + + `Read the full file (or a large enough region covering the edit) with the Read tool before retrying — ` + + `do not keep retrying Edit from a short 30–50 line window.\n` + ); } function notUniqueMessage(path: string, count: number): string { @@ -25,6 +54,10 @@ function notUniqueMessage(path: string, count: number): string { export class EditService { apply(model: TextModel, input: EditApplyInput): EditApplyResult { + if (input.allow_large_delete !== true && isOversizedEmptyDeletion(input.old_string, input.new_string)) { + return { ok: false, error: oversizedDeletionMessage(input.path) }; + } + if (input.replace_all) { const { text, count } = model.replaceAll(input.old_string, input.new_string); if (count === 0) return { ok: false, error: notFoundMessage(input.path) }; diff --git a/packages/agent-core-v2/src/app/edit/fileEdit.ts b/packages/agent-core-v2/src/app/edit/fileEdit.ts index b2b10cc7274..6cfbc67b17c 100644 --- a/packages/agent-core-v2/src/app/edit/fileEdit.ts +++ b/packages/agent-core-v2/src/app/edit/fileEdit.ts @@ -7,6 +7,7 @@ export interface FileEditInput { readonly old_string: string; readonly new_string: string; readonly replace_all: boolean; + readonly allow_large_delete?: boolean; } export type FileEditResult = diff --git a/packages/agent-core-v2/src/app/edit/fileEditService.ts b/packages/agent-core-v2/src/app/edit/fileEditService.ts index 79e8510248e..bfbc30469b0 100644 --- a/packages/agent-core-v2/src/app/edit/fileEditService.ts +++ b/packages/agent-core-v2/src/app/edit/fileEditService.ts @@ -26,6 +26,7 @@ export class FileEditService implements IFileEditService { old_string: input.old_string, new_string: input.new_string, replace_all: input.replace_all, + allow_large_delete: input.allow_large_delete, }); if (!result.ok) { return { ok: false, error: result.error }; diff --git a/packages/agent-core-v2/test/app/edit/tools/edit.test.ts b/packages/agent-core-v2/test/app/edit/tools/edit.test.ts index e77d4375ef1..cba36a6dfa4 100644 --- a/packages/agent-core-v2/test/app/edit/tools/edit.test.ts +++ b/packages/agent-core-v2/test/app/edit/tools/edit.test.ts @@ -552,6 +552,55 @@ describe('EditTool', () => { expect(writeText).toHaveBeenCalledWith('/tmp/e.txt', 'Hello !'); }); + it('refuses multi-line empty deletions unless allow_large_delete is set', async () => { + const writeText = vi.fn().mockResolvedValue(undefined); + const file = ['# Practice Phase', '', 'body line', '', 'more'].join('\n'); + const { fs } = createSpiedEditFs({ + readText: vi.fn().mockResolvedValue(file), + writeText, + }); + const tool = buildTool(fs, createTestEnv(), PERMISSIVE_WORKSPACE); + + const refused = await execute(tool, { + path: '/tmp/skill.md', + old_string: '# Practice Phase\n\nbody line', + new_string: '', + }); + expect(refused).toMatchObject({ isError: true }); + expect(refused.output).toContain('Refusing a multi-line deletion'); + expect(refused.output).toContain('allow_large_delete=true'); + expect(writeText).not.toHaveBeenCalled(); + + const allowed = await execute(tool, { + path: '/tmp/skill.md', + old_string: '# Practice Phase\n\nbody line', + new_string: '', + allow_large_delete: true, + }); + expect(allowed.output).toContain('Replaced 1 occurrence'); + expect(writeText).toHaveBeenCalledWith('/tmp/skill.md', '\n\nmore'); + }); + + it('tells the model to reread a large region when old_string is missing', async () => { + const writeText = vi.fn().mockResolvedValue(undefined); + const { fs } = createSpiedEditFs({ + readText: vi.fn().mockResolvedValue('alpha beta'), + writeText, + }); + const tool = buildTool(fs, createTestEnv(), PERMISSIVE_WORKSPACE); + + const result = await execute(tool, { + path: '/tmp/a.txt', + old_string: 'delta', + new_string: 'gamma', + }); + + expect(result).toMatchObject({ isError: true }); + expect(result.output).toContain('large enough region'); + expect(result.output).toContain('30–50 line window'); + expect(writeText).not.toHaveBeenCalled(); + }); + it('allows absolute edits outside the workspace under default policy', async () => { const writeText = vi.fn().mockResolvedValue(undefined); const { fs } = createSpiedEditFs({ diff --git a/packages/agent-core/src/tools/builtin/file/edit.md b/packages/agent-core/src/tools/builtin/file/edit.md index f928fa22fb6..808e55d826c 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.md +++ b/packages/agent-core/src/tools/builtin/file/edit.md @@ -8,6 +8,8 @@ Perform exact replacements in existing files. - If `old_string` is ambiguous, add surrounding context. Use `replace_all` only when every occurrence should change — for example, renaming a symbol throughout the file. - Multiple Edit calls may run in one response only when they do not target the same file. - DO NOT issue consecutive Edit calls on the same file. A previous Edit can invalidate a later Edit's `old_string`, causing `old_string not found`. Read the file again before the next Edit. +- After Edit fails with `old_string not found`, Read the whole file (or a large enough region covering the edit) once before retrying. Do not loop Edit → short 30–50 line Read → Edit. +- Never replace a multi-line span with an empty `new_string` unless you intend to delete it — set `allow_large_delete=true` for that intentional deletion. Prefer replacing with the real new content. - 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. diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index ed3aa8c6faa..4673e758a37 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -45,10 +45,49 @@ export const EditInputSchema = z.object({ .boolean() .optional() .describe('Set true only when every occurrence of old_string should be replaced.'), + allow_large_delete: z + .boolean() + .optional() + .describe( + 'Set true only when intentionally deleting a multi-line span with an empty (or whitespace-only) new_string. Omit for normal edits.', + ), }); export type EditInput = z.Infer; +/** Multi-line empty replacements without an explicit opt-in are refused (see #2427). */ +const LARGE_DELETE_MIN_OLD_LINES = 3; + +export function countEditLines(text: string): number { + if (text.length === 0) return 0; + let lines = 1; + for (let i = 0; i < text.length; i++) { + if (text.charCodeAt(i) === 10) lines++; + } + return lines; +} + +export function isOversizedEmptyDeletion(oldString: string, newString: string): boolean { + return newString.trim().length === 0 && countEditLines(oldString) >= LARGE_DELETE_MIN_OLD_LINES; +} + +function oversizedDeletionMessage(path: string): string { + return ( + `Refusing a multi-line deletion in ${path}: new_string is empty (or whitespace-only) while ` + + `old_string spans ${String(LARGE_DELETE_MIN_OLD_LINES)}+ lines. Read the file again, then either ` + + `replace with the intended new content, delete fewer lines at a time, or set allow_large_delete=true ` + + `if you intentionally want to remove that entire span.` + ); +} + +function notFoundMessage(path: string): string { + return ( + `old_string not found in ${path}, the file contents may be out of date. ` + + `Read the full file (or a large enough region covering the edit) with the Read tool before retrying — ` + + `do not keep retrying Edit from a short 30–50 line window.\n` + ); +} + function replaceOnceLiteral(content: string, oldString: string, newString: string): string { const index = content.indexOf(oldString); if (index === -1) return content; @@ -100,6 +139,13 @@ export class EditTool implements BuiltinTool { }; } + if ( + args.allow_large_delete !== true && + isOversizedEmptyDeletion(args.old_string, args.new_string) + ) { + return { isError: true, output: oversizedDeletionMessage(args.path) }; + } + try { const raw = await this.kaos.readText(safePath); const modelView = toModelTextView(raw); @@ -117,8 +163,7 @@ 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. -` }; + return { isError: true, output: notFoundMessage(args.path) }; } if (count > 1) { return { @@ -140,8 +185,7 @@ 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. -` }; + return { isError: true, output: notFoundMessage(args.path) }; } const newContent = parts.join(args.new_string); diff --git a/packages/agent-core/test/tools/edit.test.ts b/packages/agent-core/test/tools/edit.test.ts index 8739f8eed29..3e6bce77748 100644 --- a/packages/agent-core/test/tools/edit.test.ts +++ b/packages/agent-core/test/tools/edit.test.ts @@ -345,6 +345,64 @@ describe('EditTool', () => { expect(writeText).toHaveBeenCalledWith('/tmp/e.txt', 'Hello !'); }); + it('refuses multi-line empty deletions unless allow_large_delete is set', async () => { + const writeText = vi.fn().mockResolvedValue(0); + const file = ['# Practice Phase', '', 'body line', '', 'more'].join('\n'); + const tool = new EditTool( + createFakeKaos({ + readText: vi.fn().mockResolvedValue(file), + writeText, + }), + PERMISSIVE_WORKSPACE, + ); + + const refused = await executeTool( + tool, + context({ + path: '/tmp/skill.md', + old_string: '# Practice Phase\n\nbody line', + new_string: '', + }), + ); + expect(refused).toMatchObject({ isError: true }); + expect(refused.output).toContain('Refusing a multi-line deletion'); + expect(refused.output).toContain('allow_large_delete=true'); + expect(writeText).not.toHaveBeenCalled(); + + const allowed = await executeTool( + tool, + context({ + path: '/tmp/skill.md', + old_string: '# Practice Phase\n\nbody line', + new_string: '', + allow_large_delete: true, + }), + ); + expect(allowed.output).toContain('Replaced 1 occurrence'); + expect(writeText).toHaveBeenCalledWith('/tmp/skill.md', '\n\nmore'); + }); + + it('tells the model to reread a large region when old_string is missing', async () => { + const writeText = vi.fn().mockResolvedValue(0); + const tool = new EditTool( + createFakeKaos({ + readText: vi.fn().mockResolvedValue('alpha beta'), + writeText, + }), + PERMISSIVE_WORKSPACE, + ); + + const result = await executeTool( + tool, + context({ path: '/tmp/a.txt', old_string: 'delta', new_string: 'gamma' }), + ); + + expect(result).toMatchObject({ isError: true }); + expect(result.output).toContain('large enough region'); + expect(result.output).toContain('30–50 line window'); + expect(writeText).not.toHaveBeenCalled(); + }); + it('allows absolute edits outside the workspace under default policy', async () => { const writeText = vi.fn().mockResolvedValue(0); const tool = new EditTool( From 744fcd009285d1e361f91ebba96a6c5a019dff76 Mon Sep 17 00:00:00 2001 From: mangeshraut712 Date: Sun, 23 Aug 2026 10:17:50 +0530 Subject: [PATCH 2/2] Handle carriage-return lines in Edit guard --- .../agent-core-v2/src/app/edit/editService.ts | 4 ++-- .../test/app/edit/tools/edit.test.ts | 19 ++++++++++++++++++ .../agent-core/src/tools/builtin/file/edit.ts | 3 ++- packages/agent-core/test/tools/edit.test.ts | 20 +++++++++++++++++++ 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/packages/agent-core-v2/src/app/edit/editService.ts b/packages/agent-core-v2/src/app/edit/editService.ts index 8a3640874af..5d41aef7372 100644 --- a/packages/agent-core-v2/src/app/edit/editService.ts +++ b/packages/agent-core-v2/src/app/edit/editService.ts @@ -12,14 +12,14 @@ export type EditApplyResult = | { readonly ok: true; readonly rawContent: string; readonly count: number } | { readonly ok: false; readonly error: string }; -/** Multi-line empty replacements without an explicit opt-in are refused (see #2427). */ const LARGE_DELETE_MIN_OLD_LINES = 3; export function countEditLines(text: string): number { if (text.length === 0) return 0; let lines = 1; for (let i = 0; i < text.length; i++) { - if (text.charCodeAt(i) === 10) lines++; + const char = text[i]; + if (char === '\n' || (char === '\r' && text[i + 1] !== '\n')) lines++; } return lines; } diff --git a/packages/agent-core-v2/test/app/edit/tools/edit.test.ts b/packages/agent-core-v2/test/app/edit/tools/edit.test.ts index cba36a6dfa4..99dbbf6ced4 100644 --- a/packages/agent-core-v2/test/app/edit/tools/edit.test.ts +++ b/packages/agent-core-v2/test/app/edit/tools/edit.test.ts @@ -581,6 +581,25 @@ describe('EditTool', () => { expect(writeText).toHaveBeenCalledWith('/tmp/skill.md', '\n\nmore'); }); + it('counts lone carriage returns when guarding multi-line deletions', async () => { + const writeText = vi.fn().mockResolvedValue(undefined); + const { fs } = createSpiedEditFs({ + readText: vi.fn().mockResolvedValue('first\rsecond\rthird\rtail'), + writeText, + }); + const tool = buildTool(fs, createTestEnv(), PERMISSIVE_WORKSPACE); + + const result = await execute(tool, { + path: '/tmp/cr.txt', + old_string: 'first\rsecond\rthird', + new_string: '', + }); + + expect(result).toMatchObject({ isError: true }); + expect(result.output).toContain('Refusing a multi-line deletion'); + expect(writeText).not.toHaveBeenCalled(); + }); + it('tells the model to reread a large region when old_string is missing', async () => { const writeText = vi.fn().mockResolvedValue(undefined); const { fs } = createSpiedEditFs({ diff --git a/packages/agent-core/src/tools/builtin/file/edit.ts b/packages/agent-core/src/tools/builtin/file/edit.ts index 4673e758a37..a254fdb38f2 100644 --- a/packages/agent-core/src/tools/builtin/file/edit.ts +++ b/packages/agent-core/src/tools/builtin/file/edit.ts @@ -62,7 +62,8 @@ export function countEditLines(text: string): number { if (text.length === 0) return 0; let lines = 1; for (let i = 0; i < text.length; i++) { - if (text.charCodeAt(i) === 10) lines++; + const char = text[i]; + if (char === '\n' || (char === '\r' && text[i + 1] !== '\n')) lines++; } return lines; } diff --git a/packages/agent-core/test/tools/edit.test.ts b/packages/agent-core/test/tools/edit.test.ts index 3e6bce77748..a55b872eb30 100644 --- a/packages/agent-core/test/tools/edit.test.ts +++ b/packages/agent-core/test/tools/edit.test.ts @@ -382,6 +382,26 @@ describe('EditTool', () => { expect(writeText).toHaveBeenCalledWith('/tmp/skill.md', '\n\nmore'); }); + it('counts lone carriage returns when guarding multi-line deletions', async () => { + const writeText = vi.fn().mockResolvedValue(0); + const tool = new EditTool( + createFakeKaos({ + readText: vi.fn().mockResolvedValue('first\rsecond\rthird\rtail'), + writeText, + }), + PERMISSIVE_WORKSPACE, + ); + + const result = await executeTool( + tool, + context({ path: '/tmp/cr.txt', old_string: 'first\rsecond\rthird', new_string: '' }), + ); + + expect(result).toMatchObject({ isError: true }); + expect(result.output).toContain('Refusing a multi-line deletion'); + expect(writeText).not.toHaveBeenCalled(); + }); + it('tells the model to reread a large region when old_string is missing', async () => { const writeText = vi.fn().mockResolvedValue(0); const tool = new EditTool(