From 968e09f0b50d17f7c591baa977666b991a1e59b7 Mon Sep 17 00:00:00 2001 From: Taylor Mullen Date: Thu, 15 May 2025 23:51:53 -0700 Subject: [PATCH] fix: Ensure filename is available for diff rendering in write-file This commit resolves a bug where the `write-file` operation could fail to render content due to a missing filename. The fix involves: - Ensuring `fileName` is consistently passed to `DiffRenderer.tsx` through `ToolConfirmationMessage.tsx`, `ToolMessage.tsx`, and `useGeminiStream.ts`. - Modifying `edit.ts` and `write-file.ts` to include `fileName` in the `FileDiff` object. - Expanding the `FileDiff` interface in `tools.ts` to include `fileName`. Additionally, this commit enhances the diff rendering by: - Adding syntax highlighting based on file extension in `DiffRenderer.tsx`. - Adding more language mappings to `getLanguageFromExtension` in `DiffRenderer.tsx`. - Added lots of tests for all the above. Fixes https://b.corp.google.com/issues/418125982 --- .github/workflows/ci.yml | 6 +- package-lock.json | 19 ++ package.json | 2 +- packages/cli/package.json | 2 + .../components/messages/DiffRenderer.test.tsx | 117 +++++++ .../ui/components/messages/DiffRenderer.tsx | 33 +- .../messages/ToolConfirmationMessage.tsx | 7 +- .../ui/components/messages/ToolMessage.tsx | 5 +- .../cli/src/ui/hooks/atCommandProcessor.ts | 6 +- packages/cli/src/ui/hooks/useCompletion.ts | 12 +- packages/cli/src/ui/hooks/useGeminiStream.ts | 3 +- packages/server/package.json | 1 + packages/server/src/tools/edit.test.ts | 306 ++++++++++++++++++ packages/server/src/tools/edit.ts | 2 +- packages/server/src/tools/tools.ts | 1 + packages/server/src/tools/write-file.test.ts | 8 +- packages/server/src/tools/write-file.ts | 2 +- 17 files changed, 504 insertions(+), 28 deletions(-) create mode 100644 packages/cli/src/ui/components/messages/DiffRenderer.test.tsx create mode 100644 packages/server/src/tools/edit.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3067feea..176dca8f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,12 +38,12 @@ jobs: - name: Run linter run: npm run lint - - name: Run type check - run: npm run typecheck - - name: Build project run: npm run build + - name: Run type check + run: npm run typecheck + - name: Upload build artifacts uses: actions/upload-artifact@v4 with: diff --git a/package-lock.json b/package-lock.json index e01a0276..1255beab 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4477,6 +4477,24 @@ "react": ">=18.0.0" } }, + "node_modules/ink-testing-library": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/ink-testing-library/-/ink-testing-library-4.0.0.tgz", + "integrity": "sha512-yF92kj3pmBvk7oKbSq5vEALO//o7Z9Ck/OaLNlkzXNeYdwfpxMQkSowGTFUCS5MSu9bWfSZMewGpp7bFc66D7Q==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=18" + }, + "peerDependencies": { + "@types/react": ">=18.0.0" + }, + "peerDependenciesMeta": { + "@types/react": { + "optional": true + } + } + }, "node_modules/ink-text-input": { "version": "6.0.0", "resolved": "https://registry.npmjs.org/ink-text-input/-/ink-text-input-6.0.0.tgz", @@ -8156,6 +8174,7 @@ "@types/react": "^18.3.1", "@types/shell-quote": "^1.7.5", "@types/yargs": "^17.0.32", + "ink-testing-library": "^4.0.0", "jsdom": "^26.1.0", "typescript": "^5.3.3", "vitest": "^3.1.1" diff --git a/package.json b/package.json index 58a7e7f5..8cbba2e2 100644 --- a/package.json +++ b/package.json @@ -16,7 +16,7 @@ "debug": "NODE_ENV=development DEBUG=1 scripts/start.sh", "lint:fix": "eslint . --fix", "lint": "eslint . --ext .ts,.tsx", - "typecheck": "tsc --noEmit --jsx react-jsx", + "typecheck": "npm run typecheck --workspaces --if-present", "format": "prettier --write .", "preflight": "npm run format --workspaces --if-present && npm run lint && npm run test --workspaces --if-present", "auth:npm": "npx google-artifactregistry-auth", diff --git a/packages/cli/package.json b/packages/cli/package.json index 98d5d546..bafb2526 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -19,6 +19,7 @@ "format": "prettier --write .", "test": "vitest run", "test:ci": "vitest run --reporter=junit --outputFile=junit.xml", + "typecheck": "tsc --noEmit", "prerelease:version": "node ../../scripts/bind_package_version.js", "prerelease:deps": "node ../../scripts/bind_package_dependencies.js", "prepublishOnly": "npm publish --workspace=@gemini-code/server", @@ -52,6 +53,7 @@ "@types/react": "^18.3.1", "@types/shell-quote": "^1.7.5", "@types/yargs": "^17.0.32", + "ink-testing-library": "^4.0.0", "jsdom": "^26.1.0", "typescript": "^5.3.3", "vitest": "^3.1.1" diff --git a/packages/cli/src/ui/components/messages/DiffRenderer.test.tsx b/packages/cli/src/ui/components/messages/DiffRenderer.test.tsx new file mode 100644 index 00000000..335ee20a --- /dev/null +++ b/packages/cli/src/ui/components/messages/DiffRenderer.test.tsx @@ -0,0 +1,117 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { render } from 'ink-testing-library'; +import { DiffRenderer } from './DiffRenderer.js'; +import * as CodeColorizer from '../../utils/CodeColorizer.js'; +import { vi } from 'vitest'; + +describe('', () => { + const mockColorizeCode = vi.spyOn(CodeColorizer, 'colorizeCode'); + + beforeEach(() => { + mockColorizeCode.mockClear(); + }); + + it('should call colorizeCode with correct language for new file with known extension', () => { + const newFileDiffContent = ` +diff --git a/test.py b/test.py +new file mode 100644 +index 0000000..e69de29 +--- /dev/null ++++ b/test.py +@@ -0,0 +1 @@ ++print("hello world") +`; + render( + , + ); + expect(mockColorizeCode).toHaveBeenCalledWith( + 'print("hello world")', + 'python', + ); + }); + + it('should call colorizeCode with null language for new file with unknown extension', () => { + const newFileDiffContent = ` +diff --git a/test.unknown b/test.unknown +new file mode 100644 +index 0000000..e69de29 +--- /dev/null ++++ b/test.unknown +@@ -0,0 +1 @@ ++some content +`; + render( + , + ); + expect(mockColorizeCode).toHaveBeenCalledWith('some content', null); + }); + + it('should call colorizeCode with null language for new file if no filename is provided', () => { + const newFileDiffContent = ` +diff --git a/test.txt b/test.txt +new file mode 100644 +index 0000000..e69de29 +--- /dev/null ++++ b/test.txt +@@ -0,0 +1 @@ ++some text content +`; + render(); + expect(mockColorizeCode).toHaveBeenCalledWith('some text content', null); + }); + + it('should render diff content for existing file (not calling colorizeCode directly for the whole block)', () => { + const existingFileDiffContent = ` +diff --git a/test.txt b/test.txt +index 0000001..0000002 100644 +--- a/test.txt ++++ b/test.txt +@@ -1 +1 @@ +-old line ++new line +`; + const { lastFrame } = render( + , + ); + // colorizeCode is used internally by the line-by-line rendering, not for the whole block + expect(mockColorizeCode).not.toHaveBeenCalledWith( + expect.stringContaining('old line'), + expect.anything(), + ); + expect(mockColorizeCode).not.toHaveBeenCalledWith( + expect.stringContaining('new line'), + expect.anything(), + ); + const output = lastFrame(); + const lines = output!.split('\n'); + expect(lines[0]).toBe('1 - old line'); + expect(lines[1]).toBe('1 + new line'); + }); + + it('should handle diff with only header and no changes', () => { + const noChangeDiff = `diff --git a/file.txt b/file.txt +index 1234567..1234567 100644 +--- a/file.txt ++++ b/file.txt +`; + const { lastFrame } = render( + , + ); + expect(lastFrame()).toContain('No changes detected'); + expect(mockColorizeCode).not.toHaveBeenCalled(); + }); + + it('should handle empty diff content', () => { + const { lastFrame } = render(); + expect(lastFrame()).toContain('No diff content'); + expect(mockColorizeCode).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/ui/components/messages/DiffRenderer.tsx b/packages/cli/src/ui/components/messages/DiffRenderer.tsx index a9afeca3..4baed3e8 100644 --- a/packages/cli/src/ui/components/messages/DiffRenderer.tsx +++ b/packages/cli/src/ui/components/messages/DiffRenderer.tsx @@ -105,6 +105,14 @@ export const DiffRenderer: React.FC = ({ const parsedLines = parseDiffWithLineNumbers(diffContent); + if (parsedLines.length === 0) { + return ( + + No changes detected. + + ); + } + // Check if the diff represents a new file (only additions and header lines) const isNewFile = parsedLines.every( (line) => @@ -233,16 +241,21 @@ const renderDiffContent = ( const getLanguageFromExtension = (extension: string): string | null => { const languageMap: { [key: string]: string } = { - '.js': 'javascript', - '.ts': 'typescript', - '.py': 'python', - '.json': 'json', - '.css': 'css', - '.html': 'html', - '.sh': 'bash', - '.md': 'markdown', - '.yaml': 'yaml', - '.yml': 'yaml', + js: 'javascript', + ts: 'typescript', + py: 'python', + json: 'json', + css: 'css', + html: 'html', + sh: 'bash', + md: 'markdown', + yaml: 'yaml', + yml: 'yaml', + txt: 'plaintext', + java: 'java', + c: 'c', + cpp: 'cpp', + rb: 'ruby', }; return languageMap[extension] || null; // Return null if extension not found }; diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index b43f6843..19b1841a 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -52,7 +52,12 @@ export const ToolConfirmationMessage: React.FC< if (isEditDetails(confirmationDetails)) { // Body content is now the DiffRenderer, passing filename to it // The bordered box is removed from here and handled within DiffRenderer - bodyContent = ; + bodyContent = ( + + ); question = `Apply this change?`; options.push( diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index 091785f2..32b23b9e 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -90,7 +90,10 @@ export const ToolMessage: React.FC = ({ )} {typeof displayableResult === 'object' && ( - + )} {hiddenLines > 0 && ( diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.ts b/packages/cli/src/ui/hooks/atCommandProcessor.ts index e0cf65c5..e2934840 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.ts @@ -154,7 +154,9 @@ export async function handleAtCommand({ if (isNodeError(error) && error.code === 'ENOENT') { onDebugMessage(`Path not found, proceeding with original: ${pathSpec}`); } else { - console.error(`Error stating path ${pathPart}:`, error); + console.error( + `Error stating path ${pathPart}: ${getErrorMessage(error)}`, + ); onDebugMessage( `Error stating path, proceeding with original: ${pathSpec}`, ); @@ -200,7 +202,7 @@ export async function handleAtCommand({ ); return { processedQuery, shouldProceed: true }; - } catch (error) { + } catch (error: unknown) { // Handle errors during tool execution toolCallDisplay = { callId: `client-read-${userMessageTimestamp}`, diff --git a/packages/cli/src/ui/hooks/useCompletion.ts b/packages/cli/src/ui/hooks/useCompletion.ts index 622dc4c4..9c0c2db1 100644 --- a/packages/cli/src/ui/hooks/useCompletion.ts +++ b/packages/cli/src/ui/hooks/useCompletion.ts @@ -7,7 +7,12 @@ import { useState, useEffect, useCallback } from 'react'; import * as fs from 'fs/promises'; import * as path from 'path'; -import { isNodeError, escapePath, unescapePath } from '@gemini-code/server'; +import { + isNodeError, + escapePath, + unescapePath, + getErrorMessage, +} from '@gemini-code/server'; import { MAX_SUGGESTIONS_TO_SHOW, Suggestion, @@ -202,7 +207,7 @@ export function useCompletion( setActiveSuggestionIndex(filteredSuggestions.length > 0 ? 0 : -1); setVisibleStartIndex(0); } - } catch (error) { + } catch (error: unknown) { if (isNodeError(error) && error.code === 'ENOENT') { // Directory doesn't exist, likely mid-typing, clear suggestions if (isMounted) { @@ -211,8 +216,7 @@ export function useCompletion( } } else { console.error( - `Error fetching completion suggestions for ${baseDirAbsolute}:`, - error, + `Error fetching completion suggestions for ${baseDirAbsolute}: ${getErrorMessage(error)}`, ); if (isMounted) { resetCompletionState(); diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index a9aa8d54..29ce313d 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -326,6 +326,7 @@ export const useGeminiStream = ( if ('fileDiff' in originalDetails) { resultDisplay = { fileDiff: (originalDetails as ToolEditConfirmationDetails).fileDiff, + fileName: (originalDetails as ToolEditConfirmationDetails).fileName, }; } else { resultDisplay = `~~${(originalDetails as ToolExecuteConfirmationDetails).command}~~`; @@ -590,7 +591,7 @@ export const useGeminiStream = ( addItem( { type: MessageType.ERROR, - text: `[Stream Error: ${getErrorMessage(error)}]`, + text: `[Stream Error: ${getErrorMessage(error) || 'Unknown error'}]`, }, userMessageTimestamp, ); diff --git a/packages/server/package.json b/packages/server/package.json index c510e5ee..c90a7169 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -12,6 +12,7 @@ "format": "prettier --write .", "test": "vitest run", "test:ci": "vitest run --reporter=junit --outputFile=junit.xml", + "typecheck": "tsc --noEmit", "prerelease:version": "node ../../scripts/bind_package_version.js", "prerelease:deps": "node ../../scripts/bind_package_dependencies.js", "prepack": "npm run build" diff --git a/packages/server/src/tools/edit.test.ts b/packages/server/src/tools/edit.test.ts new file mode 100644 index 00000000..1d0db2d4 --- /dev/null +++ b/packages/server/src/tools/edit.test.ts @@ -0,0 +1,306 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/* eslint-disable @typescript-eslint/no-explicit-any */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { EditTool, EditToolParams } from './edit.js'; +import { FileDiff } from './tools.js'; +import path from 'path'; +import fs from 'fs'; +import os from 'os'; +import { Config } from '../config/config.js'; + +// Mock GeminiClient +const mockEnsureCorrectEdit = vi.fn(); +vi.mock('../core/client.js', () => ({ + GeminiClient: vi.fn().mockImplementation(() => ({ + // This is the method called by EditTool + ensureCorrectEdit: mockEnsureCorrectEdit, + })), +})); + +describe('EditTool', () => { + let tool: EditTool; + let tempDir: string; + let rootDir: string; + let mockConfig: Config; + + beforeEach(() => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'edit-tool-test-')); + rootDir = path.join(tempDir, 'root'); + fs.mkdirSync(rootDir); + + mockConfig = { + getTargetDir: () => rootDir, + getGeminiConfig: () => ({ apiKey: 'test-api-key' }), + // Add other properties/methods of Config if EditTool uses them + } as unknown as Config; + + // Reset mocks and set default implementation for ensureCorrectEdit + mockEnsureCorrectEdit.mockReset(); + mockEnsureCorrectEdit.mockImplementation(async (currentContent, params) => { + let occurrences = 0; + if (params.old_string && currentContent) { + // Simple string counting for the mock + let index = currentContent.indexOf(params.old_string); + while (index !== -1) { + occurrences++; + index = currentContent.indexOf(params.old_string, index + 1); + } + } else if (params.old_string === '') { + occurrences = 0; // Creating a new file + } + return Promise.resolve({ params, occurrences }); + }); + + tool = new EditTool(mockConfig); // GeminiClient is mocked via vi.mock + }); + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); + // vi.clearAllMocks(); // This might be too broad if other tests need persistent mocks + }); + + describe('validateParams', () => { + it('should return null for valid params', () => { + const params: EditToolParams = { + file_path: path.join(rootDir, 'test.txt'), + old_string: 'old', + new_string: 'new', + }; + expect(tool.validateParams(params)).toBeNull(); + }); + + it('should return error for relative path', () => { + const params: EditToolParams = { + file_path: 'test.txt', + old_string: 'old', + new_string: 'new', + }; + expect(tool.validateParams(params)).toMatch(/File path must be absolute/); + }); + + it('should return error for path outside root', () => { + const params: EditToolParams = { + file_path: path.join(tempDir, 'outside-root.txt'), + old_string: 'old', + new_string: 'new', + }; + expect(tool.validateParams(params)).toMatch( + /File path must be within the root directory/, + ); + }); + }); + + describe('shouldConfirmExecute', () => { + const testFile = 'edit_me.txt'; + let filePath: string; + + beforeEach(() => { + filePath = path.join(rootDir, testFile); + }); + + it('should return false if params are invalid', async () => { + const params: EditToolParams = { + file_path: 'relative.txt', + old_string: 'old', + new_string: 'new', + }; + expect(await tool.shouldConfirmExecute(params)).toBe(false); + }); + + it('should request confirmation for valid edit', async () => { + fs.writeFileSync(filePath, 'some old content here'); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + // ensureCorrectEdit will be called by shouldConfirmExecute + mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 1 }); + const confirmation = await tool.shouldConfirmExecute(params); + expect(confirmation).toEqual( + expect.objectContaining({ + title: `Confirm Edit: ${testFile}`, + fileName: testFile, + fileDiff: expect.any(String), + }), + ); + }); + + it('should return false if old_string is not found (ensureCorrectEdit returns 0)', async () => { + fs.writeFileSync(filePath, 'some content here'); + const params: EditToolParams = { + file_path: filePath, + old_string: 'not_found', + new_string: 'new', + }; + mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 0 }); + expect(await tool.shouldConfirmExecute(params)).toBe(false); + }); + + it('should return false if multiple occurrences of old_string are found (ensureCorrectEdit returns > 1)', async () => { + fs.writeFileSync(filePath, 'old old content here'); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 2 }); + expect(await tool.shouldConfirmExecute(params)).toBe(false); + }); + + it('should request confirmation for creating a new file (empty old_string)', async () => { + const newFileName = 'new_file.txt'; + const newFilePath = path.join(rootDir, newFileName); + const params: EditToolParams = { + file_path: newFilePath, + old_string: '', + new_string: 'new file content', + }; + // ensureCorrectEdit might not be called if old_string is empty, + // as shouldConfirmExecute handles this for diff generation. + // If it is called, it should return 0 occurrences for a new file. + mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 0 }); + const confirmation = await tool.shouldConfirmExecute(params); + expect(confirmation).toEqual( + expect.objectContaining({ + title: `Confirm Edit: ${newFileName}`, + fileName: newFileName, + fileDiff: expect.any(String), + }), + ); + }); + }); + + describe('execute', () => { + const testFile = 'execute_me.txt'; + let filePath: string; + + beforeEach(() => { + filePath = path.join(rootDir, testFile); + // Default for execute tests, can be overridden + mockEnsureCorrectEdit.mockImplementation(async (content, params) => { + let occurrences = 0; + if (params.old_string && content) { + let index = content.indexOf(params.old_string); + while (index !== -1) { + occurrences++; + index = content.indexOf(params.old_string, index + 1); + } + } else if (params.old_string === '') { + occurrences = 0; + } + return { params, occurrences }; + }); + }); + + it('should return error if params are invalid', async () => { + const params: EditToolParams = { + file_path: 'relative.txt', + old_string: 'old', + new_string: 'new', + }; + const result = await tool.execute(params, new AbortController().signal); + expect(result.llmContent).toMatch(/Error: Invalid parameters provided/); + expect(result.returnDisplay).toMatch(/Error: File path must be absolute/); + }); + + it('should edit an existing file and return diff with fileName', async () => { + const initialContent = 'This is some old text.'; + const newContent = 'This is some new text.'; // old -> new + fs.writeFileSync(filePath, initialContent, 'utf8'); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + + // Specific mock for this test's execution path in calculateEdit + // ensureCorrectEdit is NOT called by calculateEdit, only by shouldConfirmExecute + // So, the default mockEnsureCorrectEdit should correctly return 1 occurrence for 'old' in initialContent + + // Simulate confirmation by setting shouldAlwaysEdit + (tool as any).shouldAlwaysEdit = true; + + const result = await tool.execute(params, new AbortController().signal); + + (tool as any).shouldAlwaysEdit = false; // Reset for other tests + + expect(result.llmContent).toMatch(/Successfully modified file/); + expect(fs.readFileSync(filePath, 'utf8')).toBe(newContent); + const display = result.returnDisplay as FileDiff; + expect(display.fileDiff).toMatch(initialContent); + expect(display.fileDiff).toMatch(newContent); + expect(display.fileName).toBe(testFile); + }); + + it('should create a new file if old_string is empty and file does not exist, and return created message', async () => { + const newFileName = 'brand_new_file.txt'; + const newFilePath = path.join(rootDir, newFileName); + const fileContent = 'Content for the new file.'; + const params: EditToolParams = { + file_path: newFilePath, + old_string: '', + new_string: fileContent, + }; + + (tool as any).shouldAlwaysEdit = true; + const result = await tool.execute(params, new AbortController().signal); + (tool as any).shouldAlwaysEdit = false; + + expect(result.llmContent).toMatch(/Created new file/); + expect(fs.existsSync(newFilePath)).toBe(true); + expect(fs.readFileSync(newFilePath, 'utf8')).toBe(fileContent); + expect(result.returnDisplay).toBe(`Created ${newFileName}`); + }); + + it('should return error if old_string is not found in file', async () => { + fs.writeFileSync(filePath, 'Some content.', 'utf8'); + const params: EditToolParams = { + file_path: filePath, + old_string: 'nonexistent', + new_string: 'replacement', + }; + // The default mockEnsureCorrectEdit will return 0 occurrences for 'nonexistent' + const result = await tool.execute(params, new AbortController().signal); + expect(result.llmContent).toMatch(/0 occurrences found/); + expect(result.returnDisplay).toMatch( + /Failed to edit, could not find the string to replace/, + ); + }); + + it('should return error if multiple occurrences of old_string are found', async () => { + fs.writeFileSync(filePath, 'multiple old old strings', 'utf8'); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + // The default mockEnsureCorrectEdit will return 2 occurrences for 'old' + const result = await tool.execute(params, new AbortController().signal); + expect(result.llmContent).toMatch(/Expected 1 occurrences but found 2/); + expect(result.returnDisplay).toMatch( + /Failed to edit, expected 1 occurrence\(s\) but found 2/, + ); + }); + + it('should return error if trying to create a file that already exists (empty old_string)', async () => { + fs.writeFileSync(filePath, 'Existing content', 'utf8'); + const params: EditToolParams = { + file_path: filePath, + old_string: '', + new_string: 'new content', + }; + const result = await tool.execute(params, new AbortController().signal); + expect(result.llmContent).toMatch(/File already exists, cannot create/); + expect(result.returnDisplay).toMatch( + /Attempted to create a file that already exists/, + ); + }); + }); +}); diff --git a/packages/server/src/tools/edit.ts b/packages/server/src/tools/edit.ts index af774293..296c31ed 100644 --- a/packages/server/src/tools/edit.ts +++ b/packages/server/src/tools/edit.ts @@ -372,7 +372,7 @@ Expectation for parameters: 'Proposed', { context: 3 }, ); - displayResult = { fileDiff }; + displayResult = { fileDiff, fileName }; } const llmSuccessMessage = editData.isNewFile diff --git a/packages/server/src/tools/tools.ts b/packages/server/src/tools/tools.ts index 7bb05a95..de4bf287 100644 --- a/packages/server/src/tools/tools.ts +++ b/packages/server/src/tools/tools.ts @@ -168,6 +168,7 @@ export type ToolResultDisplay = string | FileDiff; export interface FileDiff { fileDiff: string; + fileName: string; } export interface ToolCallConfirmationDetails { diff --git a/packages/server/src/tools/write-file.test.ts b/packages/server/src/tools/write-file.test.ts index 2ce5cdf5..25d1e998 100644 --- a/packages/server/src/tools/write-file.test.ts +++ b/packages/server/src/tools/write-file.test.ts @@ -6,7 +6,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { WriteFileTool } from './write-file.js'; -import { ToolConfirmationOutcome } from './tools.js'; +import { FileDiff, ToolConfirmationOutcome } from './tools.js'; import path from 'path'; import fs from 'fs'; import os from 'os'; @@ -152,7 +152,8 @@ describe('WriteFileTool', () => { ); expect(fs.existsSync(filePath)).toBe(true); expect(fs.readFileSync(filePath, 'utf8')).toBe(content); - const display = result.returnDisplay as { fileDiff: string }; // Type assertion + const display = result.returnDisplay as FileDiff; // Type assertion + expect(display.fileName).toBe('execute_new_file.txt'); // For new files, the diff will include the filename in the "Original" header expect(display.fileDiff).toMatch(/--- execute_new_file.txt\tOriginal/); expect(display.fileDiff).toMatch(/\+\+\+ execute_new_file.txt\tWritten/); @@ -176,7 +177,8 @@ describe('WriteFileTool', () => { expect(result.llmContent).toMatch(/Successfully overwrote file/); expect(fs.readFileSync(filePath, 'utf8')).toBe(newContent); - const display = result.returnDisplay as { fileDiff: string }; // Type assertion + const display = result.returnDisplay as FileDiff; // Type assertion + expect(display.fileName).toBe('execute_existing_file.txt'); expect(display.fileDiff).toMatch(initialContent); expect(display.fileDiff).toMatch(newContent); }); diff --git a/packages/server/src/tools/write-file.ts b/packages/server/src/tools/write-file.ts index 44794a2d..21178b5b 100644 --- a/packages/server/src/tools/write-file.ts +++ b/packages/server/src/tools/write-file.ts @@ -202,7 +202,7 @@ export class WriteFileTool extends BaseTool { ? `Successfully created and wrote to new file: ${params.file_path}` : `Successfully overwrote file: ${params.file_path}`; - const displayResult: FileDiff = { fileDiff }; + const displayResult: FileDiff = { fileDiff, fileName }; return { llmContent: llmSuccessMessage,