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
116 changes: 115 additions & 1 deletion packages/core/src/tools/edit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,13 @@ describe('EditTool', () => {

beforeEach(() => {
vi.restoreAllMocks();
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'edit-tool-test-'));
// Tools resolve file paths via fs.realpathSync, so on macOS the temp dir
// under `/var/...` resolves to `/private/var/...`. Canonicalize the temp
// root up front so paths line up across config, mock workspace, and any
// sub-directories tests derive (e.g. project temp dir, plans dir).
tempDir = fs.realpathSync(
fs.mkdtempSync(path.join(os.tmpdir(), 'edit-tool-test-')),
);
rootDir = path.join(tempDir, 'root');
fs.mkdirSync(rootDir);

Expand Down Expand Up @@ -1409,4 +1415,112 @@ function doIt() {
fs.rmSync(plansDir, { recursive: true, force: true });
});
});

describe('getDescription', () => {
const testFile = 'describe_me.txt';
let filePath: string;

beforeEach(() => {
filePath = path.join(rootDir, testFile);
});

it("returns 'Create <path>' when old_string is empty", () => {
const params: EditToolParams = {
file_path: filePath,
instruction: 'Create file',
old_string: '',
new_string: 'hello',
};
expect(tool.build(params).getDescription()).toBe(`Create ${testFile}`);
});

it('returns no-change message when old_string equals new_string', () => {
const params: EditToolParams = {
file_path: filePath,
instruction: 'No-op',
old_string: 'same',
new_string: 'same',
};
expect(tool.build(params).getDescription()).toBe(
`No file changes to ${testFile}`,
);
});

it('does not append ellipsis to single-line strings within the snippet limit', () => {
const params: EditToolParams = {
file_path: filePath,
instruction: 'Short replace',
old_string: 'short old',
new_string: 'short new',
};
expect(tool.build(params).getDescription()).toBe(
`${testFile}: short old => short new`,
);
});

it('does not append ellipsis when the first line is exactly the snippet limit', () => {
const exact = 'a'.repeat(30);
const params: EditToolParams = {
file_path: filePath,
instruction: 'Boundary replace',
old_string: exact,
new_string: exact + 'b',
};
expect(tool.build(params).getDescription()).toBe(
`${testFile}: ${exact} => ${exact}...`,
);
});

it('appends ellipsis when a single-line string exceeds the snippet limit', () => {
const longOld = 'a'.repeat(35);
const longNew = 'b'.repeat(40);
const params: EditToolParams = {
file_path: filePath,
instruction: 'Long replace',
old_string: longOld,
new_string: longNew,
};
expect(tool.build(params).getDescription()).toBe(
`${testFile}: ${'a'.repeat(30)}... => ${'b'.repeat(30)}...`,
);
});

// Regression for #28110 and #28109: when an edit spans multiple lines but the
// first line is short, the snippet should still signal hidden content with `...`.
it('appends ellipsis to multi-line edits whose first line is short', () => {
const oldStr = 'function foo() {\n return 1;\n}';
const newStr = 'function foo() {\n return 2;\n}';
const params: EditToolParams = {
file_path: filePath,
instruction: 'Multi-line replace with short first line',
old_string: oldStr,
new_string: newStr,
};
expect(tool.build(params).getDescription()).toBe(
`${testFile}: function foo() {... => function foo() {...`,
);
});

it('appends ellipsis to multi-line edits with very short first lines', () => {
const params: EditToolParams = {
file_path: filePath,
instruction: 'Tiny multi-line',
old_string: 'a\nb\nc',
new_string: 'x\ny\nz',
};
expect(tool.build(params).getDescription()).toBe(
`${testFile}: a... => x...`,
);
});

it('emits an empty snippet without ellipsis when deleting to empty', () => {
const params: EditToolParams = {
file_path: filePath,
instruction: 'Delete content',
old_string: 'a\nb\nc',
new_string: '',
};
expect(tool.build(params).getDescription()).toBe(`${testFile}: a... => `);
});
});
});
15 changes: 9 additions & 6 deletions packages/core/src/tools/edit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -837,12 +837,15 @@ class EditToolInvocation
return `Create ${shortenPath(relativePath)}`;
}

const oldStringSnippet =
this.params.old_string.split('\n')[0].substring(0, 30) +
(this.params.old_string.length > 30 ? '...' : '');
const newStringSnippet =
this.params.new_string.split('\n')[0].substring(0, 30) +
(this.params.new_string.length > 30 ? '...' : '');
// Append `...` when the snippet hides content from the user: the first
// line was truncated, or there are additional lines below it.
const snippet = (s: string) => {
const firstLine = s.split('\n')[0];
const hasHiddenContent = firstLine.length > 30 || s.includes('\n');
return firstLine.substring(0, 30) + (hasHiddenContent ? '...' : '');
};
Comment on lines +842 to +846

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When splitting the string by \n on files with CRLF (\r\n) line endings, the resulting firstLine will retain the trailing carriage return (\r). Since this is a terminal-first CLI tool, printing a string containing a carriage return followed by an ellipsis (e.g., foo\r...) can cause the terminal cursor to reset to the beginning of the line, leading to rendering corruption where the ellipsis overwrites the start of the line.

Additionally, when truncating strings that may contain multi-byte Unicode characters (such as emojis), we should use methods that operate on grapheme clusters (like Array.from()) instead of UTF-16 code units (string.length, string.substring()) to prevent character splitting.

Suggested change
const snippet = (s: string) => {
const firstLine = s.split('\n')[0];
const hasHiddenContent = firstLine.length > 30 || s.includes('\n');
return firstLine.substring(0, 30) + (hasHiddenContent ? '...' : '');
};
const snippet = (s: string) => {
const firstLine = s.split(/\r?\n/)[0];
const chars = Array.from(firstLine);
const hasHiddenContent = chars.length > 30 || s.includes('\n');
return chars.slice(0, 30).join('') + (hasHiddenContent ? '...' : '');
};
References
  1. When truncating strings that may contain multi-byte Unicode characters (e.g., emojis), use methods that operate on grapheme clusters (like Intl.Segmenter or Array.from()) instead of UTF-16 code units (string.length, string.slice()) to prevent character splitting.

const oldStringSnippet = snippet(this.params.old_string);
const newStringSnippet = snippet(this.params.new_string);

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