mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-12 07:01:09 -07:00
fix(core): prevent omission placeholder deletions in replace/write_file (#19870)
Co-authored-by: Bryan Morgan <bryanmorgan@google.com>
This commit is contained in:
@@ -547,7 +547,7 @@ A good instruction should concisely answer:
|
||||
"type": "string",
|
||||
},
|
||||
"new_string": {
|
||||
"description": "The exact literal text to replace \`old_string\` with, preferably unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic.",
|
||||
"description": "The exact literal text to replace \`old_string\` with, preferably unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic. Do not use omission placeholders like '(rest of methods ...)', '...', or 'unchanged code'; provide exact literal code.",
|
||||
"type": "string",
|
||||
},
|
||||
"old_string": {
|
||||
@@ -665,7 +665,7 @@ exports[`coreTools snapshots for specific models > Model: gemini-2.5-pro > snaps
|
||||
"parametersJsonSchema": {
|
||||
"properties": {
|
||||
"content": {
|
||||
"description": "The content to write to the file.",
|
||||
"description": "The content to write to the file. Do not use omission placeholders like '(rest of methods ...)', '...', or 'unchanged code'; provide complete literal content.",
|
||||
"type": "string",
|
||||
},
|
||||
"file_path": {
|
||||
@@ -1312,7 +1312,7 @@ The user has the ability to modify the \`new_string\` content. If modified, this
|
||||
"type": "string",
|
||||
},
|
||||
"new_string": {
|
||||
"description": "The exact literal text to replace \`old_string\` with, unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic.",
|
||||
"description": "The exact literal text to replace \`old_string\` with, unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic. Do not use omission placeholders like '(rest of methods ...)', '...', or 'unchanged code'; provide exact literal code.",
|
||||
"type": "string",
|
||||
},
|
||||
"old_string": {
|
||||
@@ -1429,7 +1429,7 @@ The user has the ability to modify \`content\`. If modified, this will be stated
|
||||
"parametersJsonSchema": {
|
||||
"properties": {
|
||||
"content": {
|
||||
"description": "The content to write to the file.",
|
||||
"description": "The content to write to the file. Do not use omission placeholders like '(rest of methods ...)', '...', or 'unchanged code'; provide complete literal content.",
|
||||
"type": "string",
|
||||
},
|
||||
"file_path": {
|
||||
|
||||
@@ -71,7 +71,8 @@ export const DEFAULT_LEGACY_SET: CoreToolSet = {
|
||||
type: 'string',
|
||||
},
|
||||
content: {
|
||||
description: 'The content to write to the file.',
|
||||
description:
|
||||
"The content to write to the file. Do not use omission placeholders like '(rest of methods ...)', '...', or 'unchanged code'; provide complete literal content.",
|
||||
type: 'string',
|
||||
},
|
||||
},
|
||||
@@ -332,7 +333,7 @@ A good instruction should concisely answer:
|
||||
},
|
||||
new_string: {
|
||||
description:
|
||||
'The exact literal text to replace `old_string` with, preferably unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic.',
|
||||
"The exact literal text to replace `old_string` with, preferably unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic. Do not use omission placeholders like '(rest of methods ...)', '...', or 'unchanged code'; provide exact literal code.",
|
||||
type: 'string',
|
||||
},
|
||||
expected_replacements: {
|
||||
|
||||
@@ -73,7 +73,8 @@ The user has the ability to modify \`content\`. If modified, this will be stated
|
||||
type: 'string',
|
||||
},
|
||||
content: {
|
||||
description: 'The content to write to the file.',
|
||||
description:
|
||||
"The content to write to the file. Do not use omission placeholders like '(rest of methods ...)', '...', or 'unchanged code'; provide complete literal content.",
|
||||
type: 'string',
|
||||
},
|
||||
},
|
||||
@@ -310,7 +311,7 @@ The user has the ability to modify the \`new_string\` content. If modified, this
|
||||
},
|
||||
new_string: {
|
||||
description:
|
||||
'The exact literal text to replace `old_string` with, unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic.',
|
||||
"The exact literal text to replace `old_string` with, unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic. Do not use omission placeholders like '(rest of methods ...)', '...', or 'unchanged code'; provide exact literal code.",
|
||||
type: 'string',
|
||||
},
|
||||
expected_replacements: {
|
||||
|
||||
@@ -607,6 +607,53 @@ function doIt() {
|
||||
};
|
||||
expect(tool.validateToolParams(params)).toMatch(/Path not in workspace/);
|
||||
});
|
||||
|
||||
it('should reject omission placeholder in new_string when old_string does not contain that placeholder', () => {
|
||||
const params: EditToolParams = {
|
||||
file_path: path.join(rootDir, 'test.txt'),
|
||||
instruction: 'An instruction',
|
||||
old_string: 'old content',
|
||||
new_string: '(rest of methods ...)',
|
||||
};
|
||||
expect(tool.validateToolParams(params)).toBe(
|
||||
"`new_string` contains an omission placeholder (for example 'rest of methods ...'). Provide exact literal replacement text.",
|
||||
);
|
||||
});
|
||||
|
||||
it('should reject new_string when it contains an additional placeholder not present in old_string', () => {
|
||||
const params: EditToolParams = {
|
||||
file_path: path.join(rootDir, 'test.txt'),
|
||||
instruction: 'An instruction',
|
||||
old_string: '(rest of methods ...)',
|
||||
new_string: `(rest of methods ...)
|
||||
(unchanged code ...)`,
|
||||
};
|
||||
expect(tool.validateToolParams(params)).toBe(
|
||||
"`new_string` contains an omission placeholder (for example 'rest of methods ...'). Provide exact literal replacement text.",
|
||||
);
|
||||
});
|
||||
|
||||
it('should allow omission placeholders when all are already present in old_string', () => {
|
||||
const params: EditToolParams = {
|
||||
file_path: path.join(rootDir, 'test.txt'),
|
||||
instruction: 'An instruction',
|
||||
old_string: `(rest of methods ...)
|
||||
(unchanged code ...)`,
|
||||
new_string: `(unchanged code ...)
|
||||
(rest of methods ...)`,
|
||||
};
|
||||
expect(tool.validateToolParams(params)).toBeNull();
|
||||
});
|
||||
|
||||
it('should allow normal code that contains placeholder text in a string literal', () => {
|
||||
const params: EditToolParams = {
|
||||
file_path: path.join(rootDir, 'test.ts'),
|
||||
instruction: 'Update string literal',
|
||||
old_string: 'const msg = "old";',
|
||||
new_string: 'const msg = "(rest of methods ...)";',
|
||||
};
|
||||
expect(tool.validateToolParams(params)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('execute', () => {
|
||||
|
||||
@@ -54,6 +54,7 @@ import { debugLogger } from '../utils/debugLogger.js';
|
||||
import levenshtein from 'fast-levenshtein';
|
||||
import { EDIT_DEFINITION } from './definitions/coreTools.js';
|
||||
import { resolveToolDeclaration } from './definitions/resolver.js';
|
||||
import { detectOmissionPlaceholders } from './omissionPlaceholderDetector.js';
|
||||
|
||||
const ENABLE_FUZZY_MATCH_RECOVERY = true;
|
||||
const FUZZY_MATCH_THRESHOLD = 0.1; // Allow up to 10% weighted difference
|
||||
@@ -973,6 +974,19 @@ export class EditTool
|
||||
}
|
||||
params.file_path = filePath;
|
||||
|
||||
const newPlaceholders = detectOmissionPlaceholders(params.new_string);
|
||||
if (newPlaceholders.length > 0) {
|
||||
const oldPlaceholders = new Set(
|
||||
detectOmissionPlaceholders(params.old_string),
|
||||
);
|
||||
|
||||
for (const placeholder of newPlaceholders) {
|
||||
if (!oldPlaceholders.has(placeholder)) {
|
||||
return "`new_string` contains an omission placeholder (for example 'rest of methods ...'). Provide exact literal replacement text.";
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return this.config.validatePathAccess(params.file_path);
|
||||
}
|
||||
|
||||
|
||||
63
packages/core/src/tools/omissionPlaceholderDetector.test.ts
Normal file
63
packages/core/src/tools/omissionPlaceholderDetector.test.ts
Normal file
@@ -0,0 +1,63 @@
|
||||
/**
|
||||
* @license
|
||||
* Copyright 2025 Google LLC
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import { detectOmissionPlaceholders } from './omissionPlaceholderDetector.js';
|
||||
|
||||
describe('detectOmissionPlaceholders', () => {
|
||||
it('detects standalone placeholder lines', () => {
|
||||
expect(detectOmissionPlaceholders('(rest of methods ...)')).toEqual([
|
||||
'rest of methods ...',
|
||||
]);
|
||||
expect(detectOmissionPlaceholders('(rest of code ...)')).toEqual([
|
||||
'rest of code ...',
|
||||
]);
|
||||
expect(detectOmissionPlaceholders('(unchanged code ...)')).toEqual([
|
||||
'unchanged code ...',
|
||||
]);
|
||||
expect(detectOmissionPlaceholders('// rest of methods ...')).toEqual([
|
||||
'rest of methods ...',
|
||||
]);
|
||||
});
|
||||
|
||||
it('detects case-insensitive placeholders', () => {
|
||||
expect(detectOmissionPlaceholders('(Rest Of Methods ...)')).toEqual([
|
||||
'rest of methods ...',
|
||||
]);
|
||||
});
|
||||
|
||||
it('detects multiple placeholder lines in one input', () => {
|
||||
const text = `class Example {
|
||||
run() {}
|
||||
(rest of methods ...)
|
||||
(unchanged code ...)
|
||||
}`;
|
||||
expect(detectOmissionPlaceholders(text)).toEqual([
|
||||
'rest of methods ...',
|
||||
'unchanged code ...',
|
||||
]);
|
||||
});
|
||||
|
||||
it('does not detect placeholders embedded in normal code', () => {
|
||||
expect(
|
||||
detectOmissionPlaceholders(
|
||||
'const note = "(rest of methods ...)";\nconsole.log(note);',
|
||||
),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('does not detect omission phrase when inline in a comment', () => {
|
||||
expect(
|
||||
detectOmissionPlaceholders('return value; // rest of methods ...'),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('does not detect unrelated ellipsis text', () => {
|
||||
expect(detectOmissionPlaceholders('const message = "loading...";')).toEqual(
|
||||
[],
|
||||
);
|
||||
});
|
||||
});
|
||||
106
packages/core/src/tools/omissionPlaceholderDetector.ts
Normal file
106
packages/core/src/tools/omissionPlaceholderDetector.ts
Normal file
@@ -0,0 +1,106 @@
|
||||
/**
|
||||
* @license
|
||||
* Copyright 2025 Google LLC
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
const OMITTED_PREFIXES = new Set([
|
||||
'rest of',
|
||||
'rest of method',
|
||||
'rest of methods',
|
||||
'rest of code',
|
||||
'unchanged code',
|
||||
'unchanged method',
|
||||
'unchanged methods',
|
||||
]);
|
||||
|
||||
function isAllDots(str: string): boolean {
|
||||
if (str.length === 0) {
|
||||
return false;
|
||||
}
|
||||
for (let i = 0; i < str.length; i++) {
|
||||
if (str[i] !== '.') {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
function normalizeWhitespace(input: string): string {
|
||||
const segments: string[] = [];
|
||||
let current = '';
|
||||
|
||||
for (const char of input) {
|
||||
if (char === ' ' || char === '\t' || char === '\n' || char === '\r') {
|
||||
if (current.length > 0) {
|
||||
segments.push(current);
|
||||
current = '';
|
||||
}
|
||||
continue;
|
||||
}
|
||||
current += char;
|
||||
}
|
||||
|
||||
if (current.length > 0) {
|
||||
segments.push(current);
|
||||
}
|
||||
|
||||
return segments.join(' ');
|
||||
}
|
||||
|
||||
function normalizePlaceholder(line: string): string | null {
|
||||
let text = line.trim();
|
||||
if (!text) {
|
||||
return null;
|
||||
}
|
||||
|
||||
if (text.startsWith('//')) {
|
||||
text = text.slice(2).trim();
|
||||
}
|
||||
|
||||
if (text.startsWith('(') && text.endsWith(')')) {
|
||||
text = text.slice(1, -1).trim();
|
||||
}
|
||||
|
||||
const ellipsisStart = text.indexOf('...');
|
||||
if (ellipsisStart < 0) {
|
||||
return null;
|
||||
}
|
||||
|
||||
const prefixRaw = text.slice(0, ellipsisStart).trim().toLowerCase();
|
||||
const suffixRaw = text.slice(ellipsisStart + 3).trim();
|
||||
const prefix = normalizeWhitespace(prefixRaw);
|
||||
|
||||
if (!OMITTED_PREFIXES.has(prefix)) {
|
||||
return null;
|
||||
}
|
||||
|
||||
if (suffixRaw.length > 0 && !isAllDots(suffixRaw)) {
|
||||
return null;
|
||||
}
|
||||
|
||||
return `${prefix} ...`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Detects shorthand omission placeholders such as:
|
||||
* - (rest of methods ...)
|
||||
* - (rest of code ...)
|
||||
* - (unchanged code ...)
|
||||
* - // rest of methods ...
|
||||
*
|
||||
* Returns all placeholders found as normalized tokens.
|
||||
*/
|
||||
export function detectOmissionPlaceholders(text: string): string[] {
|
||||
const lines = text.replaceAll('\r\n', '\n').split('\n');
|
||||
const matches: string[] = [];
|
||||
|
||||
for (const rawLine of lines) {
|
||||
const normalized = normalizePlaceholder(rawLine);
|
||||
if (normalized) {
|
||||
matches.push(normalized);
|
||||
}
|
||||
}
|
||||
|
||||
return matches;
|
||||
}
|
||||
@@ -310,6 +310,42 @@ describe('WriteFileTool', () => {
|
||||
};
|
||||
expect(() => tool.build(params)).toThrow(`Missing or empty "file_path"`);
|
||||
});
|
||||
|
||||
it('should throw an error if content includes an omission placeholder', () => {
|
||||
const params = {
|
||||
file_path: path.join(rootDir, 'placeholder.txt'),
|
||||
content: '(rest of methods ...)',
|
||||
};
|
||||
expect(() => tool.build(params)).toThrow(
|
||||
"`content` contains an omission placeholder (for example 'rest of methods ...'). Provide complete file content.",
|
||||
);
|
||||
});
|
||||
|
||||
it('should throw an error when multiline content includes omission placeholders', () => {
|
||||
const params = {
|
||||
file_path: path.join(rootDir, 'service.ts'),
|
||||
content: `class Service {
|
||||
execute() {
|
||||
return "run";
|
||||
}
|
||||
|
||||
// rest of methods ...
|
||||
}`,
|
||||
};
|
||||
expect(() => tool.build(params)).toThrow(
|
||||
"`content` contains an omission placeholder (for example 'rest of methods ...'). Provide complete file content.",
|
||||
);
|
||||
});
|
||||
|
||||
it('should allow content with placeholder text in a normal string literal', () => {
|
||||
const params = {
|
||||
file_path: path.join(rootDir, 'valid-content.ts'),
|
||||
content: 'const note = "(rest of methods ...)";',
|
||||
};
|
||||
const invocation = tool.build(params);
|
||||
expect(invocation).toBeDefined();
|
||||
expect(invocation.params).toEqual(params);
|
||||
});
|
||||
});
|
||||
|
||||
describe('getCorrectedFileContent', () => {
|
||||
|
||||
@@ -47,6 +47,7 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
import { debugLogger } from '../utils/debugLogger.js';
|
||||
import { WRITE_FILE_DEFINITION } from './definitions/coreTools.js';
|
||||
import { resolveToolDeclaration } from './definitions/resolver.js';
|
||||
import { detectOmissionPlaceholders } from './omissionPlaceholderDetector.js';
|
||||
|
||||
/**
|
||||
* Parameters for the WriteFile tool
|
||||
@@ -486,6 +487,11 @@ export class WriteFileTool
|
||||
}`;
|
||||
}
|
||||
|
||||
const omissionPlaceholders = detectOmissionPlaceholders(params.content);
|
||||
if (omissionPlaceholders.length > 0) {
|
||||
return "`content` contains an omission placeholder (for example 'rest of methods ...'). Provide complete file content.";
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user