Skip to content

Commit e530737

Browse files
MegashubhamHehehe
authored andcommitted
fix(core): correct ellipsis logic in EditTool getDescription()
The description snippet for old_string and new_string was using the total string length to decide whether to append '...' instead of checking the first-line length that was actually being displayed. This caused two issues: - Multi-line strings with a short first line (< 30 chars) would not show '...' even though the edit description was only showing the first line, hiding that the edit spans multiple lines. - The check was inconsistent: substring(0, 30) was applied on the first line, but the length guard was on the entire string. Fix by extracting the first line into a variable and checking: 1. Whether the first line itself exceeds 30 characters (truncated by substring), OR 2. Whether the full string is multi-line (\n present), indicating more content exists beyond what is shown. Added unit tests to cover all cases: new file, short single-line, multi-line with short first line, long first line, and no-op edits.
1 parent be7ba2c commit e530737

2 files changed

Lines changed: 85 additions & 6 deletions

File tree

packages/core/src/tools/edit.test.ts

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,79 @@ function doIt() {
703703
});
704704
});
705705

706+
describe('getDescription', () => {
707+
it('should return "Create <path>" for new file (empty old_string)', () => {
708+
const filePath = path.join(rootDir, 'new.ts');
709+
const params: EditToolParams = {
710+
file_path: filePath,
711+
instruction: 'Create new file',
712+
old_string: '',
713+
new_string: 'content',
714+
};
715+
const invocation = tool.build(params);
716+
expect(invocation.getDescription()).toMatch(/^Create /);
717+
});
718+
719+
it('should NOT append "..." to a short single-line old_string and new_string', () => {
720+
const filePath = path.join(rootDir, 'test.ts');
721+
const params: EditToolParams = {
722+
file_path: filePath,
723+
instruction: 'Replace short old with short new',
724+
old_string: 'short old',
725+
new_string: 'short new',
726+
};
727+
const invocation = tool.build(params);
728+
const description = invocation.getDescription();
729+
expect(description).toContain('short old');
730+
expect(description).toContain('short new');
731+
expect(description).not.toMatch(/short old\.\.\./);
732+
expect(description).not.toMatch(/short new\.\.\./);
733+
});
734+
735+
it('should append "..." when old_string or new_string is multi-line, even if first line is short', () => {
736+
const filePath = path.join(rootDir, 'test.ts');
737+
// old_string first line is only 17 chars (< 30), but the full string is multi-line
738+
const params: EditToolParams = {
739+
file_path: filePath,
740+
instruction: 'Replace function foo with bar',
741+
old_string: 'function foo() {\n return 1;\n}',
742+
new_string: 'function bar() {\n return 2;\n}',
743+
};
744+
const invocation = tool.build(params);
745+
const description = invocation.getDescription();
746+
// First lines are short, but since strings are multi-line, '...' should appear
747+
// The full first line 'function foo() {' is kept (17 chars < 30) and '...' is appended
748+
expect(description).toContain('function foo() {...');
749+
expect(description).toContain('function bar() {...');
750+
});
751+
752+
it('should append "..." when the first line of old_string exceeds 30 characters', () => {
753+
const filePath = path.join(rootDir, 'test.ts');
754+
const longFirstLine = 'a'.repeat(35); // 35 chars, exceeds 30
755+
const params: EditToolParams = {
756+
file_path: filePath,
757+
instruction: 'Replace long line',
758+
old_string: longFirstLine,
759+
new_string: 'short',
760+
};
761+
const invocation = tool.build(params);
762+
const description = invocation.getDescription();
763+
expect(description).toContain('...');
764+
});
765+
766+
it('should return "No file changes" when old_string equals new_string', () => {
767+
const filePath = path.join(rootDir, 'test.ts');
768+
const params: EditToolParams = {
769+
file_path: filePath,
770+
instruction: 'No-op edit',
771+
old_string: 'same content',
772+
new_string: 'same content',
773+
};
774+
const invocation = tool.build(params);
775+
expect(invocation.getDescription()).toMatch(/No file changes/);
776+
});
777+
});
778+
706779
describe('execute', () => {
707780
const testFile = 'execute_me.txt';
708781
let filePath: string;

packages/core/src/tools/edit.ts

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -837,12 +837,18 @@ class EditToolInvocation
837837
return `Create ${shortenPath(relativePath)}`;
838838
}
839839

840-
const oldStringSnippet =
841-
this.params.old_string.split('\n')[0].substring(0, 30) +
842-
(this.params.old_string.length > 30 ? '...' : '');
843-
const newStringSnippet =
844-
this.params.new_string.split('\n')[0].substring(0, 30) +
845-
(this.params.new_string.length > 30 ? '...' : '');
840+
const getSnippet = (str: string): string => {
841+
const newlineIdx = str.indexOf('\n');
842+
const firstLine = newlineIdx !== -1 ? str.substring(0, newlineIdx) : str;
843+
const chars = Array.from(firstLine);
844+
if (chars.length > 30) {
845+
return chars.slice(0, 30).join('') + '...';
846+
}
847+
return firstLine + (newlineIdx !== -1 ? '...' : '');
848+
};
849+
850+
const oldStringSnippet = getSnippet(this.params.old_string);
851+
const newStringSnippet = getSnippet(this.params.new_string);
846852

847853
if (this.params.old_string === this.params.new_string) {
848854
return `No file changes to ${shortenPath(relativePath)}`;

0 commit comments

Comments
 (0)