Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/edit-refuse-large-delete.md
Original file line number Diff line number Diff line change
@@ -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`.
2 changes: 2 additions & 0 deletions packages/agent-core-v2/src/agent/tools/edit/edit.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
6 changes: 6 additions & 0 deletions packages/agent-core-v2/src/agent/tools/edit/edit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
mangeshraut712 marked this conversation as resolved.
.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<typeof EditInputSchema>;
Expand Down
1 change: 1 addition & 0 deletions packages/agent-core-v2/src/agent/tools/edit/editTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down
37 changes: 35 additions & 2 deletions packages/agent-core-v2/src/app/edit/editService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };

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++) {
const char = text[i];
if (char === '\n' || (char === '\r' && text[i + 1] !== '\n')) lines++;
}
Comment thread
mangeshraut712 marked this conversation as resolved.
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 {
Expand All @@ -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) };
Expand Down
1 change: 1 addition & 0 deletions packages/agent-core-v2/src/app/edit/fileEdit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
1 change: 1 addition & 0 deletions packages/agent-core-v2/src/app/edit/fileEditService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down
68 changes: 68 additions & 0 deletions packages/agent-core-v2/test/app/edit/tools/edit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,74 @@ 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('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({
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({
Expand Down
2 changes: 2 additions & 0 deletions packages/agent-core/src/tools/builtin/file/edit.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
53 changes: 49 additions & 4 deletions packages/agent-core/src/tools/builtin/file/edit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,50 @@ 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<typeof EditInputSchema>;

/** 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++) {
const char = text[i];
if (char === '\n' || (char === '\r' && text[i + 1] !== '\n')) 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;
Expand Down Expand Up @@ -100,6 +140,13 @@ export class EditTool implements BuiltinTool<EditInput> {
};
}

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);
Expand All @@ -117,8 +164,7 @@ export class EditTool implements BuiltinTool<EditInput> {
}

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 {
Expand All @@ -140,8 +186,7 @@ export class EditTool implements BuiltinTool<EditInput> {
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);
Expand Down
78 changes: 78 additions & 0 deletions packages/agent-core/test/tools/edit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,84 @@ 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('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(
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(
Expand Down