From 695785c69d5c2a7a24ef13306ecdb63ef7c92116 Mon Sep 17 00:00:00 2001 From: Thomas Shephard Date: Fri, 30 Jan 2026 00:57:06 +0000 Subject: [PATCH] feat: preserve EOL in files (#16087) Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Adib234 <30782825+Adib234@users.noreply.github.com> Co-authored-by: Jack Wotherspoon --- packages/core/src/tools/edit.ts | 24 +- packages/core/src/tools/line-endings.test.ts | 282 ++++++++++++++++++ packages/core/src/tools/write-file.test.ts | 1 + packages/core/src/tools/write-file.ts | 14 +- packages/core/src/utils/editCorrector.test.ts | 27 ++ packages/core/src/utils/editCorrector.ts | 12 +- packages/core/src/utils/textUtils.ts | 11 + 7 files changed, 353 insertions(+), 18 deletions(-) create mode 100644 packages/core/src/tools/line-endings.test.ts diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index baaefe9a92..40ae914f50 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -6,6 +6,7 @@ import * as fsPromises from 'node:fs/promises'; import * as path from 'node:path'; +import * as os from 'node:os'; import * as crypto from 'node:crypto'; import * as Diff from 'diff'; import { @@ -34,7 +35,7 @@ import { } from './modifiable-tool.js'; import { IdeClient } from '../ide/ide-client.js'; import { FixLLMEditWithInstruction } from '../utils/llm-edit-fixer.js'; -import { safeLiteralReplace } from '../utils/textUtils.js'; +import { safeLiteralReplace, detectLineEnding } from '../utils/textUtils.js'; import { EditStrategyEvent } from '../telemetry/types.js'; import { logEditStrategy } from '../telemetry/loggers.js'; import { EditCorrectionEvent } from '../telemetry/types.js'; @@ -258,17 +259,6 @@ async function calculateRegexReplacement( }; } -/** - * Detects the line ending style of a string. - * @param content The string content to analyze. - * @returns '\r\n' for Windows-style, '\n' for Unix-style. - */ -function detectLineEnding(content: string): '\r\n' | '\n' { - // If a Carriage Return is found, assume Windows-style endings. - // This is a simple but effective heuristic. - return content.includes('\r\n') ? '\r\n' : '\n'; -} - export async function calculateReplacement( config: Config, context: ReplacementContext, @@ -812,9 +802,13 @@ class EditToolInvocation await this.ensureParentDirectoriesExistAsync(this.params.file_path); let finalContent = editData.newContent; - // Restore original line endings if they were CRLF - if (!editData.isNewFile && editData.originalLineEnding === '\r\n') { - finalContent = finalContent.replace(/\n/g, '\r\n'); + // Restore original line endings if they were CRLF, or use OS default for new files + const useCRLF = + (!editData.isNewFile && editData.originalLineEnding === '\r\n') || + (editData.isNewFile && os.EOL === '\r\n'); + + if (useCRLF) { + finalContent = finalContent.replace(/\r?\n/g, '\r\n'); } await this.config .getFileSystemService() diff --git a/packages/core/src/tools/line-endings.test.ts b/packages/core/src/tools/line-endings.test.ts new file mode 100644 index 0000000000..f62d684712 --- /dev/null +++ b/packages/core/src/tools/line-endings.test.ts @@ -0,0 +1,282 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + describe, + it, + expect, + beforeEach, + afterEach, + vi, + type Mocked, +} from 'vitest'; +import { detectLineEnding } from '../utils/textUtils.js'; +import { WriteFileTool } from './write-file.js'; +import { EditTool } from './edit.js'; +import type { Config } from '../config/config.js'; +import { ApprovalMode } from '../policy/types.js'; +import { ToolConfirmationOutcome } from './tools.js'; +import type { ToolRegistry } from './tool-registry.js'; +import path from 'node:path'; +import fs from 'node:fs'; +import os from 'node:os'; +import { GeminiClient } from '../core/client.js'; +import type { BaseLlmClient } from '../core/baseLlmClient.js'; +import { + ensureCorrectEdit, + ensureCorrectFileContent, +} from '../utils/editCorrector.js'; +import { StandardFileSystemService } from '../services/fileSystemService.js'; +import { WorkspaceContext } from '../utils/workspaceContext.js'; +import { + createMockMessageBus, + getMockMessageBusInstance, +} from '../test-utils/mock-message-bus.js'; + +const rootDir = path.resolve(os.tmpdir(), 'gemini-cli-line-ending-test-root'); + +// --- MOCKS --- +vi.mock('../core/client.js'); +vi.mock('../utils/editCorrector.js'); +vi.mock('../ide/ide-client.js', () => ({ + IdeClient: { + getInstance: vi.fn().mockResolvedValue({ + openDiff: vi.fn(), + isDiffingEnabled: vi.fn().mockReturnValue(false), + }), + }, +})); + +let mockGeminiClientInstance: Mocked; +let mockBaseLlmClientInstance: Mocked; +const mockEnsureCorrectEdit = vi.fn(); +const mockEnsureCorrectFileContent = vi.fn(); + +// Mock Config +const fsService = new StandardFileSystemService(); +const mockConfigInternal = { + getTargetDir: () => rootDir, + getApprovalMode: vi.fn(() => ApprovalMode.DEFAULT), + setApprovalMode: vi.fn(), + getGeminiClient: vi.fn(), + getBaseLlmClient: vi.fn(), + getFileSystemService: () => fsService, + getIdeMode: vi.fn(() => false), + getWorkspaceContext: () => new WorkspaceContext(rootDir), + getApiKey: () => 'test-key', + getModel: () => 'test-model', + getSandbox: () => false, + getDebugMode: () => false, + getQuestion: () => undefined, + getToolDiscoveryCommand: () => undefined, + getToolCallCommand: () => undefined, + getMcpServerCommand: () => undefined, + getMcpServers: () => undefined, + getUserAgent: () => 'test-agent', + getUserMemory: () => '', + setUserMemory: vi.fn(), + getGeminiMdFileCount: () => 0, + setGeminiMdFileCount: vi.fn(), + getDisableLLMCorrection: vi.fn(() => false), + validatePathAccess: vi.fn().mockReturnValue(null), + getToolRegistry: () => + ({ + registerTool: vi.fn(), + discoverTools: vi.fn(), + }) as unknown as ToolRegistry, + isInteractive: () => false, +}; +const mockConfig = mockConfigInternal as unknown as Config; + +vi.mock('../telemetry/loggers.js', () => ({ + logFileOperation: vi.fn(), + logEditStrategy: vi.fn(), + logEditCorrectionEvent: vi.fn(), +})); + +// --- END MOCKS --- + +describe('Line Ending Preservation', () => { + let tempDir: string; + + beforeEach(() => { + vi.clearAllMocks(); + tempDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'line-ending-test-external-'), + ); + if (!fs.existsSync(rootDir)) { + fs.mkdirSync(rootDir, { recursive: true }); + } + + mockGeminiClientInstance = new (vi.mocked(GeminiClient))( + mockConfig, + ) as Mocked; + vi.mocked(GeminiClient).mockImplementation(() => mockGeminiClientInstance); + + mockBaseLlmClientInstance = { + generateJson: vi.fn(), + } as unknown as Mocked; + + vi.mocked(ensureCorrectEdit).mockImplementation(mockEnsureCorrectEdit); + vi.mocked(ensureCorrectFileContent).mockImplementation( + mockEnsureCorrectFileContent, + ); + + mockConfigInternal.getGeminiClient.mockReturnValue( + mockGeminiClientInstance, + ); + mockConfigInternal.getBaseLlmClient.mockReturnValue( + mockBaseLlmClientInstance, + ); + }); + + afterEach(() => { + if (fs.existsSync(tempDir)) { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + if (fs.existsSync(rootDir)) { + fs.rmSync(rootDir, { recursive: true, force: true }); + } + vi.restoreAllMocks(); + }); + + describe('detectLineEnding', () => { + it('should detect CRLF', () => { + expect(detectLineEnding('line1\r\nline2')).toBe('\r\n'); + expect(detectLineEnding('line1\r\n')).toBe('\r\n'); + }); + + it('should detect LF', () => { + expect(detectLineEnding('line1\nline2')).toBe('\n'); + expect(detectLineEnding('line1\n')).toBe('\n'); + expect(detectLineEnding('line1')).toBe('\n'); // Default to LF if no newline + }); + }); + + describe('WriteFileTool', () => { + let tool: WriteFileTool; + const abortSignal = new AbortController().signal; + + beforeEach(() => { + const bus = createMockMessageBus(); + getMockMessageBusInstance(bus).defaultToolDecision = 'ask_user'; + tool = new WriteFileTool(mockConfig, bus); + }); + + it('should preserve CRLF when overwriting an existing file', async () => { + const filePath = path.join(rootDir, 'crlf_file.txt'); + const originalContent = 'line1\r\nline2\r\n'; + fs.writeFileSync(filePath, originalContent); // Write with CRLF (or however Node writes binary buffer) + // Ensure strictly CRLF + fs.writeFileSync(filePath, Buffer.from('line1\r\nline2\r\n')); + + // Proposed content from LLM (usually LF) + const proposedContent = 'line1\nline2\nline3\n'; + + // Mock corrections to return proposed content as-is (but usually normalized) + mockEnsureCorrectEdit.mockResolvedValue({ + params: { + file_path: filePath, + old_string: originalContent, + new_string: proposedContent, + }, + occurrences: 1, + }); + + const params = { file_path: filePath, content: proposedContent }; + const invocation = tool.build(params); + + // Force approval + const confirmDetails = await invocation.shouldConfirmExecute(abortSignal); + if ( + confirmDetails && + typeof confirmDetails === 'object' && + 'onConfirm' in confirmDetails + ) { + await confirmDetails.onConfirm(ToolConfirmationOutcome.ProceedOnce); + } + + await invocation.execute(abortSignal); + + const writtenContent = fs.readFileSync(filePath, 'utf8'); + // Expect all newlines to be CRLF + expect(writtenContent).toBe('line1\r\nline2\r\nline3\r\n'); + }); + + it('should use OS EOL for new files', async () => { + const filePath = path.join(rootDir, 'new_os_eol_file.txt'); + const proposedContent = 'line1\nline2\n'; + + mockEnsureCorrectFileContent.mockResolvedValue(proposedContent); + + const params = { file_path: filePath, content: proposedContent }; + const invocation = tool.build(params); + + const confirmDetails = await invocation.shouldConfirmExecute(abortSignal); + if ( + confirmDetails && + typeof confirmDetails === 'object' && + 'onConfirm' in confirmDetails + ) { + await confirmDetails.onConfirm(ToolConfirmationOutcome.ProceedOnce); + } + + await invocation.execute(abortSignal); + + const writtenContent = fs.readFileSync(filePath, 'utf8'); + + if (os.EOL === '\r\n') { + expect(writtenContent).toBe('line1\r\nline2\r\n'); + } else { + expect(writtenContent).toBe('line1\nline2\n'); + } + }); + }); + + describe('EditTool', () => { + let tool: EditTool; + const abortSignal = new AbortController().signal; + + beforeEach(() => { + const bus = createMockMessageBus(); + getMockMessageBusInstance(bus).defaultToolDecision = 'ask_user'; + tool = new EditTool(mockConfig, bus); + }); + + it('should preserve CRLF when editing a file', async () => { + const filePath = path.join(rootDir, 'edit_crlf.txt'); + const originalContent = 'line1\r\nline2\r\nline3\r\n'; + fs.writeFileSync(filePath, Buffer.from(originalContent)); + + const oldString = 'line2'; + const newString = 'modified'; + + const params = { + file_path: filePath, + old_string: oldString, + new_string: newString, + instruction: 'Change line2 to modified', + }; + const invocation = tool.build(params); + + // Force approval + const confirmDetails = await invocation.shouldConfirmExecute(abortSignal); + if ( + confirmDetails && + typeof confirmDetails === 'object' && + 'onConfirm' in confirmDetails + ) { + await confirmDetails.onConfirm(ToolConfirmationOutcome.ProceedOnce); + } + + await invocation.execute(abortSignal); + + const writtenContent = fs.readFileSync(filePath, 'utf8'); + + expect(writtenContent).toBe('line1\r\nmodified\r\nline3\r\n'); + }); + }); +}); diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 2a0d3301be..3545affe3f 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -901,6 +901,7 @@ describe('WriteFileTool', () => { errorMessage: 'Generic write error', expectedMessagePrefix: 'Error writing to file', mockFsExistsSync: false, + restoreAllMocks: false, }, ])( 'should return $errorType error when write fails with $errorCode', diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 4cc6f5998d..8dfc4d7855 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -7,6 +7,7 @@ import fs from 'node:fs'; import fsPromises from 'node:fs/promises'; import path from 'node:path'; +import os from 'node:os'; import * as Diff from 'diff'; import { WRITE_FILE_TOOL_NAME } from './tool-names.js'; import type { Config } from '../config/config.js'; @@ -33,6 +34,7 @@ import { ensureCorrectEdit, ensureCorrectFileContent, } from '../utils/editCorrector.js'; +import { detectLineEnding } from '../utils/textUtils.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; import type { ModifiableDeclarativeTool, @@ -301,9 +303,19 @@ class WriteFileToolInvocation extends BaseToolInvocation< await fsPromises.mkdir(dirName, { recursive: true }); } + let finalContent = fileContent; + const useCRLF = + !isNewFile && originalContent + ? detectLineEnding(originalContent) === '\r\n' + : os.EOL === '\r\n'; + + if (useCRLF) { + finalContent = finalContent.replace(/\r?\n/g, '\r\n'); + } + await this.config .getFileSystemService() - .writeTextFile(this.resolvedPath, fileContent); + .writeTextFile(this.resolvedPath, finalContent); // Generate diff for display result const fileName = path.basename(this.resolvedPath); diff --git a/packages/core/src/utils/editCorrector.test.ts b/packages/core/src/utils/editCorrector.test.ts index 2d3f4b9a20..8695b488e8 100644 --- a/packages/core/src/utils/editCorrector.test.ts +++ b/packages/core/src/utils/editCorrector.test.ts @@ -639,6 +639,33 @@ describe('editCorrector', () => { expect(result.params).toEqual(originalParams); }); }); + + describe('Scenario Group 7: Trimming with Newline Preservation', () => { + it('Test 7.1: should preserve trailing newlines in new_string when trimming is applied', async () => { + const currentContent = ' find me'; // Matches old_string initially + const originalParams = { + file_path: '/test/file.txt', + old_string: ' find me', // Matches, but has whitespace to trim + new_string: ' replaced\n\n', // Needs trimming but preserve newlines + }; + + const result = await ensureCorrectEdit( + '/test/file.txt', + currentContent, + originalParams, + mockGeminiClientInstance, + mockBaseLlmClientInstance, + abortSignal, + false, + ); + + // old_string should be trimmed to 'find me' because 'find me' also exists uniquely in ' find me' + expect(result.params.old_string).toBe('find me'); + // new_string should be trimmed of spaces but keep ALL newlines + expect(result.params.new_string).toBe('replaced\n\n'); + expect(result.occurrences).toBe(1); + }); + }); }); describe('ensureCorrectFileContent', () => { diff --git a/packages/core/src/utils/editCorrector.ts b/packages/core/src/utils/editCorrector.ts index 4762f0c91f..d61628ee4f 100644 --- a/packages/core/src/utils/editCorrector.ts +++ b/packages/core/src/utils/editCorrector.ts @@ -689,13 +689,20 @@ Return ONLY the corrected string in the specified JSON format with the key 'corr } } +function trimPreservingTrailingNewline(str: string): string { + const trimmedEnd = str.trimEnd(); + const trailingWhitespace = str.slice(trimmedEnd.length); + const trailingNewlines = trailingWhitespace.replace(/[^\r\n]/g, ''); + return str.trim() + trailingNewlines; +} + function trimPairIfPossible( target: string, trimIfTargetTrims: string, currentContent: string, expectedReplacements: number, ) { - const trimmedTargetString = target.trim(); + const trimmedTargetString = trimPreservingTrailingNewline(target); if (target.length !== trimmedTargetString.length) { const trimmedTargetOccurrences = countOccurrences( currentContent, @@ -703,7 +710,8 @@ function trimPairIfPossible( ); if (trimmedTargetOccurrences === expectedReplacements) { - const trimmedReactiveString = trimIfTargetTrims.trim(); + const trimmedReactiveString = + trimPreservingTrailingNewline(trimIfTargetTrims); return { targetString: trimmedTargetString, pair: trimmedReactiveString, diff --git a/packages/core/src/utils/textUtils.ts b/packages/core/src/utils/textUtils.ts index c227d89e98..525637f1e2 100644 --- a/packages/core/src/utils/textUtils.ts +++ b/packages/core/src/utils/textUtils.ts @@ -54,6 +54,17 @@ export function isBinary( return false; } +/** + * Detects the line ending style of a string. + * @param content The string content to analyze. + * @returns '\r\n' for Windows-style, '\n' for Unix-style. + */ +export function detectLineEnding(content: string): '\r\n' | '\n' { + // If a Carriage Return is found, assume Windows-style endings. + // This is a simple but effective heuristic. + return content.includes('\r\n') ? '\r\n' : '\n'; +} + /** * Truncates a string to a maximum length, appending a suffix if truncated. * @param str The string to truncate.