Skip to content
Merged
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
109 changes: 83 additions & 26 deletions packages/core/src/utils/editHelper.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ describe('normalizeEditStrings', () => {
const two = 2;
`;

it('returns literal matches unchanged and trims new_string trailing whitespace', () => {
it('returns literal matches unchanged', () => {
const result = normalizeEditStrings(
file,
'const two = 2;',
' const two = 42; ',
' const two = 42;',
);
expect(result).toEqual({
oldString: 'const two = 2;',
Expand All @@ -32,11 +32,11 @@ const two = 2;
const result = normalizeEditStrings(
"const greeting = 'Don't';\n",
'const greeting = β€˜Don’t’;',
'const greeting = β€œHello”; ',
'const greeting = "Hello";',
);
expect(result).toEqual({
oldString: "const greeting = 'Don't';",
newString: 'const greeting = β€œHello”;',
newString: 'const greeting = "Hello";',
});
});

Expand All @@ -48,35 +48,15 @@ const two = 2;
});
});

it('still trims new_string when editing a brand-new file', () => {
const result = normalizeEditStrings(null, '', 'new file contents ');
expect(result).toEqual({
oldString: '',
newString: 'new file contents',
});
});

it('matches unicode dash variants', () => {
it('matches unicode dash variants and preserves newString', () => {
const result = normalizeEditStrings(
'const range = "1-2";\n',
'const range = "1\u20132";',
'const range = "3\u20135"; ',
);
expect(result).toEqual({
oldString: 'const range = "1-2";',
newString: 'const range = "3\u20135";',
});
});

it('matches when trailing whitespace differs only at line ends', () => {
const result = normalizeEditStrings(
'value = 1;\n',
'value = 1; \n',
'value = 2; \n',
);
expect(result).toEqual({
oldString: 'value = 1;\n',
newString: 'value = 2;\n',
newString: 'const range = "3\u20135"; ',
});
});

Expand All @@ -103,6 +83,83 @@ const two = 2;
newString: 'console.log("bye")',
});
});

// Tests for issue #1618: Preserve trailing whitespace in newString
describe('trailing whitespace preservation in newString', () => {
it('preserves trailing whitespace when intentionally adding to end of line', () => {
// Test with tab
const result1 = normalizeEditStrings(
'value = 1;\n',
'value = 1;\n',
'value = 1;\t\n',
);
expect(result1.newString).toBe('value = 1;\t\n');

// Test with spaces (same behavior, just different whitespace char)
const result2 = normalizeEditStrings('text\n', 'text\n', 'text \n');
expect(result2.newString).toBe('text \n');
});

it('preserves newString trailing whitespace even when oldString is fuzzy matched', () => {
const result = normalizeEditStrings(
'value = 1;\n', // File has no trailing spaces
'value = 1; \n', // LLM copied with extra spaces (will be fuzzy matched)
'value = 2; \n', // LLM replacement also has spaces
);
expect(result).toEqual({
oldString: 'value = 1;\n', // Canonical from file
newString: 'value = 2; \n', // Preserved as LLM intended
});
});

it('preserves trailing whitespace in multi-line template literals', () => {
const file = 'const s = "";\n';
const result = normalizeEditStrings(
file,
'const s = "";',
'const s = `line1 \nline2`;', // Trailing spaces after line1 are significant
);
expect(result.newString).toBe('const s = `line1 \nline2`;');
});

it('preserves trailing whitespace when creating new file', () => {
const result = normalizeEditStrings(
null,
'',
'content with trailing tab\t\n',
);
expect(result).toEqual({
oldString: '',
newString: 'content with trailing tab\t\n',
});
});

it('still supports fuzzy matching after trailing whitespace was added in previous edit', () => {
// Round 1: Add trailing spaces to a line
let fileContent = 'value = 1;\n';
const round1 = normalizeEditStrings(
fileContent,
'value = 1;\n',
'value = 1; \n', // Adding trailing spaces
);
expect(round1.newString).toBe('value = 1; \n');

// Simulate the edit being applied
fileContent = fileContent.replace(round1.oldString, round1.newString);
expect(fileContent).toBe('value = 1; \n'); // File now has trailing spaces

// Round 2: LLM tries to edit again, but its oldString doesn't have trailing spaces
// (because LLM context may not preserve exact whitespace)
const round2 = normalizeEditStrings(
fileContent,
'value = 1;\n', // LLM thinks there's no trailing spaces
'value = 2;\n',
);
// Fuzzy matching should still find the line and return canonical slice WITH trailing spaces
expect(round2.oldString).toBe('value = 1; \n');
expect(round2.newString).toBe('value = 2;\n');
});
});
});

describe('countOccurrences', () => {
Expand Down
43 changes: 11 additions & 32 deletions packages/core/src/utils/editHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,30 +64,6 @@ function normalizeBasicCharacters(text: string): string {
return normalized;
}

/**
* Removes trailing whitespace from each line while keeping the original newline
* separators intact.
*/
function stripTrailingWhitespacePreserveNewlines(text: string): string {
const pieces = text.split(/(\r\n|\n|\r)/);
let result = '';

for (let i = 0; i < pieces.length; i++) {
const segment = pieces[i];
if (segment === undefined) {
continue;
}

if (i % 2 === 0) {
result += segment.trimEnd();
} else {
result += segment;
}
}

return result;
}

/* -------------------------------------------------------------------------- */
/* Line-based search helpers */
/* -------------------------------------------------------------------------- */
Expand Down Expand Up @@ -323,23 +299,26 @@ export interface NormalizedEditStrings {

/**
* Runs the core normalization pipeline:
* 1. Strip trailing whitespace copied from numbered output.
* 2. Attempt to find the literal text inside {@link fileContent}.
* 3. If found through a relaxed match (smart quotes, line trims, etc.),
* 1. Attempt to find the literal text inside {@link fileContent}.
* 2. If found through a relaxed match (smart quotes, line trims, etc.),
* return the canonical slice from disk so later replacements operate on
* exact bytes.
* 3. Preserve newString as-is (it represents the LLM's intent).
*
* Note: Trailing whitespace in newString is intentionally NOT stripped.
* While LLMs may sometimes accidentally add trailing whitespace, stripping it
* unconditionally breaks legitimate use cases where trailing whitespace is
* intentional (e.g., multi-line strings, heredocs). See issue #1618.
*/
export function normalizeEditStrings(
fileContent: string | null,
oldString: string,
newString: string,
): NormalizedEditStrings {
const trimmedNewString = stripTrailingWhitespacePreserveNewlines(newString);

if (fileContent === null || oldString === '') {
return {
oldString,
newString: trimmedNewString,
newString,
};
}

Expand All @@ -348,15 +327,15 @@ export function normalizeEditStrings(
return {
oldString: canonicalOriginal.slice,
newString: adjustNewStringForTrailingLine(
trimmedNewString,
newString,
canonicalOriginal.removedTrailingFinalEmptyLine,
),
};
}

return {
oldString,
newString: trimmedNewString,
newString,
};
}

Expand Down