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
This commit is contained in:
parent
dce7d2c4f7
commit
968e09f0b5
|
@ -38,12 +38,12 @@ jobs:
|
||||||
- name: Run linter
|
- name: Run linter
|
||||||
run: npm run lint
|
run: npm run lint
|
||||||
|
|
||||||
- name: Run type check
|
|
||||||
run: npm run typecheck
|
|
||||||
|
|
||||||
- name: Build project
|
- name: Build project
|
||||||
run: npm run build
|
run: npm run build
|
||||||
|
|
||||||
|
- name: Run type check
|
||||||
|
run: npm run typecheck
|
||||||
|
|
||||||
- name: Upload build artifacts
|
- name: Upload build artifacts
|
||||||
uses: actions/upload-artifact@v4
|
uses: actions/upload-artifact@v4
|
||||||
with:
|
with:
|
||||||
|
|
|
@ -4477,6 +4477,24 @@
|
||||||
"react": ">=18.0.0"
|
"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": {
|
"node_modules/ink-text-input": {
|
||||||
"version": "6.0.0",
|
"version": "6.0.0",
|
||||||
"resolved": "https://registry.npmjs.org/ink-text-input/-/ink-text-input-6.0.0.tgz",
|
"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/react": "^18.3.1",
|
||||||
"@types/shell-quote": "^1.7.5",
|
"@types/shell-quote": "^1.7.5",
|
||||||
"@types/yargs": "^17.0.32",
|
"@types/yargs": "^17.0.32",
|
||||||
|
"ink-testing-library": "^4.0.0",
|
||||||
"jsdom": "^26.1.0",
|
"jsdom": "^26.1.0",
|
||||||
"typescript": "^5.3.3",
|
"typescript": "^5.3.3",
|
||||||
"vitest": "^3.1.1"
|
"vitest": "^3.1.1"
|
||||||
|
|
|
@ -16,7 +16,7 @@
|
||||||
"debug": "NODE_ENV=development DEBUG=1 scripts/start.sh",
|
"debug": "NODE_ENV=development DEBUG=1 scripts/start.sh",
|
||||||
"lint:fix": "eslint . --fix",
|
"lint:fix": "eslint . --fix",
|
||||||
"lint": "eslint . --ext .ts,.tsx",
|
"lint": "eslint . --ext .ts,.tsx",
|
||||||
"typecheck": "tsc --noEmit --jsx react-jsx",
|
"typecheck": "npm run typecheck --workspaces --if-present",
|
||||||
"format": "prettier --write .",
|
"format": "prettier --write .",
|
||||||
"preflight": "npm run format --workspaces --if-present && npm run lint && npm run test --workspaces --if-present",
|
"preflight": "npm run format --workspaces --if-present && npm run lint && npm run test --workspaces --if-present",
|
||||||
"auth:npm": "npx google-artifactregistry-auth",
|
"auth:npm": "npx google-artifactregistry-auth",
|
||||||
|
|
|
@ -19,6 +19,7 @@
|
||||||
"format": "prettier --write .",
|
"format": "prettier --write .",
|
||||||
"test": "vitest run",
|
"test": "vitest run",
|
||||||
"test:ci": "vitest run --reporter=junit --outputFile=junit.xml",
|
"test:ci": "vitest run --reporter=junit --outputFile=junit.xml",
|
||||||
|
"typecheck": "tsc --noEmit",
|
||||||
"prerelease:version": "node ../../scripts/bind_package_version.js",
|
"prerelease:version": "node ../../scripts/bind_package_version.js",
|
||||||
"prerelease:deps": "node ../../scripts/bind_package_dependencies.js",
|
"prerelease:deps": "node ../../scripts/bind_package_dependencies.js",
|
||||||
"prepublishOnly": "npm publish --workspace=@gemini-code/server",
|
"prepublishOnly": "npm publish --workspace=@gemini-code/server",
|
||||||
|
@ -52,6 +53,7 @@
|
||||||
"@types/react": "^18.3.1",
|
"@types/react": "^18.3.1",
|
||||||
"@types/shell-quote": "^1.7.5",
|
"@types/shell-quote": "^1.7.5",
|
||||||
"@types/yargs": "^17.0.32",
|
"@types/yargs": "^17.0.32",
|
||||||
|
"ink-testing-library": "^4.0.0",
|
||||||
"jsdom": "^26.1.0",
|
"jsdom": "^26.1.0",
|
||||||
"typescript": "^5.3.3",
|
"typescript": "^5.3.3",
|
||||||
"vitest": "^3.1.1"
|
"vitest": "^3.1.1"
|
||||||
|
|
|
@ -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('<DiffRenderer />', () => {
|
||||||
|
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(
|
||||||
|
<DiffRenderer diffContent={newFileDiffContent} filename="test.py" />,
|
||||||
|
);
|
||||||
|
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(
|
||||||
|
<DiffRenderer diffContent={newFileDiffContent} filename="test.unknown" />,
|
||||||
|
);
|
||||||
|
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(<DiffRenderer diffContent={newFileDiffContent} />);
|
||||||
|
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(
|
||||||
|
<DiffRenderer
|
||||||
|
diffContent={existingFileDiffContent}
|
||||||
|
filename="test.txt"
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
// 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(
|
||||||
|
<DiffRenderer diffContent={noChangeDiff} filename="file.txt" />,
|
||||||
|
);
|
||||||
|
expect(lastFrame()).toContain('No changes detected');
|
||||||
|
expect(mockColorizeCode).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should handle empty diff content', () => {
|
||||||
|
const { lastFrame } = render(<DiffRenderer diffContent="" />);
|
||||||
|
expect(lastFrame()).toContain('No diff content');
|
||||||
|
expect(mockColorizeCode).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
});
|
|
@ -105,6 +105,14 @@ export const DiffRenderer: React.FC<DiffRendererProps> = ({
|
||||||
|
|
||||||
const parsedLines = parseDiffWithLineNumbers(diffContent);
|
const parsedLines = parseDiffWithLineNumbers(diffContent);
|
||||||
|
|
||||||
|
if (parsedLines.length === 0) {
|
||||||
|
return (
|
||||||
|
<Box borderStyle="round" borderColor={Colors.SubtleComment} padding={1}>
|
||||||
|
<Text dimColor>No changes detected.</Text>
|
||||||
|
</Box>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
// Check if the diff represents a new file (only additions and header lines)
|
// Check if the diff represents a new file (only additions and header lines)
|
||||||
const isNewFile = parsedLines.every(
|
const isNewFile = parsedLines.every(
|
||||||
(line) =>
|
(line) =>
|
||||||
|
@ -233,16 +241,21 @@ const renderDiffContent = (
|
||||||
|
|
||||||
const getLanguageFromExtension = (extension: string): string | null => {
|
const getLanguageFromExtension = (extension: string): string | null => {
|
||||||
const languageMap: { [key: string]: string } = {
|
const languageMap: { [key: string]: string } = {
|
||||||
'.js': 'javascript',
|
js: 'javascript',
|
||||||
'.ts': 'typescript',
|
ts: 'typescript',
|
||||||
'.py': 'python',
|
py: 'python',
|
||||||
'.json': 'json',
|
json: 'json',
|
||||||
'.css': 'css',
|
css: 'css',
|
||||||
'.html': 'html',
|
html: 'html',
|
||||||
'.sh': 'bash',
|
sh: 'bash',
|
||||||
'.md': 'markdown',
|
md: 'markdown',
|
||||||
'.yaml': 'yaml',
|
yaml: 'yaml',
|
||||||
'.yml': 'yaml',
|
yml: 'yaml',
|
||||||
|
txt: 'plaintext',
|
||||||
|
java: 'java',
|
||||||
|
c: 'c',
|
||||||
|
cpp: 'cpp',
|
||||||
|
rb: 'ruby',
|
||||||
};
|
};
|
||||||
return languageMap[extension] || null; // Return null if extension not found
|
return languageMap[extension] || null; // Return null if extension not found
|
||||||
};
|
};
|
||||||
|
|
|
@ -52,7 +52,12 @@ export const ToolConfirmationMessage: React.FC<
|
||||||
if (isEditDetails(confirmationDetails)) {
|
if (isEditDetails(confirmationDetails)) {
|
||||||
// Body content is now the DiffRenderer, passing filename to it
|
// Body content is now the DiffRenderer, passing filename to it
|
||||||
// The bordered box is removed from here and handled within DiffRenderer
|
// The bordered box is removed from here and handled within DiffRenderer
|
||||||
bodyContent = <DiffRenderer diffContent={confirmationDetails.fileDiff} />;
|
bodyContent = (
|
||||||
|
<DiffRenderer
|
||||||
|
diffContent={confirmationDetails.fileDiff}
|
||||||
|
filename={confirmationDetails.fileName}
|
||||||
|
/>
|
||||||
|
);
|
||||||
|
|
||||||
question = `Apply this change?`;
|
question = `Apply this change?`;
|
||||||
options.push(
|
options.push(
|
||||||
|
|
|
@ -90,7 +90,10 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({
|
||||||
</Box>
|
</Box>
|
||||||
)}
|
)}
|
||||||
{typeof displayableResult === 'object' && (
|
{typeof displayableResult === 'object' && (
|
||||||
<DiffRenderer diffContent={displayableResult.fileDiff} />
|
<DiffRenderer
|
||||||
|
diffContent={displayableResult.fileDiff}
|
||||||
|
filename={displayableResult.fileName}
|
||||||
|
/>
|
||||||
)}
|
)}
|
||||||
{hiddenLines > 0 && (
|
{hiddenLines > 0 && (
|
||||||
<Box>
|
<Box>
|
||||||
|
|
|
@ -154,7 +154,9 @@ export async function handleAtCommand({
|
||||||
if (isNodeError(error) && error.code === 'ENOENT') {
|
if (isNodeError(error) && error.code === 'ENOENT') {
|
||||||
onDebugMessage(`Path not found, proceeding with original: ${pathSpec}`);
|
onDebugMessage(`Path not found, proceeding with original: ${pathSpec}`);
|
||||||
} else {
|
} else {
|
||||||
console.error(`Error stating path ${pathPart}:`, error);
|
console.error(
|
||||||
|
`Error stating path ${pathPart}: ${getErrorMessage(error)}`,
|
||||||
|
);
|
||||||
onDebugMessage(
|
onDebugMessage(
|
||||||
`Error stating path, proceeding with original: ${pathSpec}`,
|
`Error stating path, proceeding with original: ${pathSpec}`,
|
||||||
);
|
);
|
||||||
|
@ -200,7 +202,7 @@ export async function handleAtCommand({
|
||||||
);
|
);
|
||||||
|
|
||||||
return { processedQuery, shouldProceed: true };
|
return { processedQuery, shouldProceed: true };
|
||||||
} catch (error) {
|
} catch (error: unknown) {
|
||||||
// Handle errors during tool execution
|
// Handle errors during tool execution
|
||||||
toolCallDisplay = {
|
toolCallDisplay = {
|
||||||
callId: `client-read-${userMessageTimestamp}`,
|
callId: `client-read-${userMessageTimestamp}`,
|
||||||
|
|
|
@ -7,7 +7,12 @@
|
||||||
import { useState, useEffect, useCallback } from 'react';
|
import { useState, useEffect, useCallback } from 'react';
|
||||||
import * as fs from 'fs/promises';
|
import * as fs from 'fs/promises';
|
||||||
import * as path from 'path';
|
import * as path from 'path';
|
||||||
import { isNodeError, escapePath, unescapePath } from '@gemini-code/server';
|
import {
|
||||||
|
isNodeError,
|
||||||
|
escapePath,
|
||||||
|
unescapePath,
|
||||||
|
getErrorMessage,
|
||||||
|
} from '@gemini-code/server';
|
||||||
import {
|
import {
|
||||||
MAX_SUGGESTIONS_TO_SHOW,
|
MAX_SUGGESTIONS_TO_SHOW,
|
||||||
Suggestion,
|
Suggestion,
|
||||||
|
@ -202,7 +207,7 @@ export function useCompletion(
|
||||||
setActiveSuggestionIndex(filteredSuggestions.length > 0 ? 0 : -1);
|
setActiveSuggestionIndex(filteredSuggestions.length > 0 ? 0 : -1);
|
||||||
setVisibleStartIndex(0);
|
setVisibleStartIndex(0);
|
||||||
}
|
}
|
||||||
} catch (error) {
|
} catch (error: unknown) {
|
||||||
if (isNodeError(error) && error.code === 'ENOENT') {
|
if (isNodeError(error) && error.code === 'ENOENT') {
|
||||||
// Directory doesn't exist, likely mid-typing, clear suggestions
|
// Directory doesn't exist, likely mid-typing, clear suggestions
|
||||||
if (isMounted) {
|
if (isMounted) {
|
||||||
|
@ -211,8 +216,7 @@ export function useCompletion(
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
console.error(
|
console.error(
|
||||||
`Error fetching completion suggestions for ${baseDirAbsolute}:`,
|
`Error fetching completion suggestions for ${baseDirAbsolute}: ${getErrorMessage(error)}`,
|
||||||
error,
|
|
||||||
);
|
);
|
||||||
if (isMounted) {
|
if (isMounted) {
|
||||||
resetCompletionState();
|
resetCompletionState();
|
||||||
|
|
|
@ -326,6 +326,7 @@ export const useGeminiStream = (
|
||||||
if ('fileDiff' in originalDetails) {
|
if ('fileDiff' in originalDetails) {
|
||||||
resultDisplay = {
|
resultDisplay = {
|
||||||
fileDiff: (originalDetails as ToolEditConfirmationDetails).fileDiff,
|
fileDiff: (originalDetails as ToolEditConfirmationDetails).fileDiff,
|
||||||
|
fileName: (originalDetails as ToolEditConfirmationDetails).fileName,
|
||||||
};
|
};
|
||||||
} else {
|
} else {
|
||||||
resultDisplay = `~~${(originalDetails as ToolExecuteConfirmationDetails).command}~~`;
|
resultDisplay = `~~${(originalDetails as ToolExecuteConfirmationDetails).command}~~`;
|
||||||
|
@ -590,7 +591,7 @@ export const useGeminiStream = (
|
||||||
addItem(
|
addItem(
|
||||||
{
|
{
|
||||||
type: MessageType.ERROR,
|
type: MessageType.ERROR,
|
||||||
text: `[Stream Error: ${getErrorMessage(error)}]`,
|
text: `[Stream Error: ${getErrorMessage(error) || 'Unknown error'}]`,
|
||||||
},
|
},
|
||||||
userMessageTimestamp,
|
userMessageTimestamp,
|
||||||
);
|
);
|
||||||
|
|
|
@ -12,6 +12,7 @@
|
||||||
"format": "prettier --write .",
|
"format": "prettier --write .",
|
||||||
"test": "vitest run",
|
"test": "vitest run",
|
||||||
"test:ci": "vitest run --reporter=junit --outputFile=junit.xml",
|
"test:ci": "vitest run --reporter=junit --outputFile=junit.xml",
|
||||||
|
"typecheck": "tsc --noEmit",
|
||||||
"prerelease:version": "node ../../scripts/bind_package_version.js",
|
"prerelease:version": "node ../../scripts/bind_package_version.js",
|
||||||
"prerelease:deps": "node ../../scripts/bind_package_dependencies.js",
|
"prerelease:deps": "node ../../scripts/bind_package_dependencies.js",
|
||||||
"prepack": "npm run build"
|
"prepack": "npm run build"
|
||||||
|
|
|
@ -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/,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
|
@ -372,7 +372,7 @@ Expectation for parameters:
|
||||||
'Proposed',
|
'Proposed',
|
||||||
{ context: 3 },
|
{ context: 3 },
|
||||||
);
|
);
|
||||||
displayResult = { fileDiff };
|
displayResult = { fileDiff, fileName };
|
||||||
}
|
}
|
||||||
|
|
||||||
const llmSuccessMessage = editData.isNewFile
|
const llmSuccessMessage = editData.isNewFile
|
||||||
|
|
|
@ -168,6 +168,7 @@ export type ToolResultDisplay = string | FileDiff;
|
||||||
|
|
||||||
export interface FileDiff {
|
export interface FileDiff {
|
||||||
fileDiff: string;
|
fileDiff: string;
|
||||||
|
fileName: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface ToolCallConfirmationDetails {
|
export interface ToolCallConfirmationDetails {
|
||||||
|
|
|
@ -6,7 +6,7 @@
|
||||||
|
|
||||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||||
import { WriteFileTool } from './write-file.js';
|
import { WriteFileTool } from './write-file.js';
|
||||||
import { ToolConfirmationOutcome } from './tools.js';
|
import { FileDiff, ToolConfirmationOutcome } from './tools.js';
|
||||||
import path from 'path';
|
import path from 'path';
|
||||||
import fs from 'fs';
|
import fs from 'fs';
|
||||||
import os from 'os';
|
import os from 'os';
|
||||||
|
@ -152,7 +152,8 @@ describe('WriteFileTool', () => {
|
||||||
);
|
);
|
||||||
expect(fs.existsSync(filePath)).toBe(true);
|
expect(fs.existsSync(filePath)).toBe(true);
|
||||||
expect(fs.readFileSync(filePath, 'utf8')).toBe(content);
|
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
|
// 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\tOriginal/);
|
||||||
expect(display.fileDiff).toMatch(/\+\+\+ execute_new_file.txt\tWritten/);
|
expect(display.fileDiff).toMatch(/\+\+\+ execute_new_file.txt\tWritten/);
|
||||||
|
@ -176,7 +177,8 @@ describe('WriteFileTool', () => {
|
||||||
|
|
||||||
expect(result.llmContent).toMatch(/Successfully overwrote file/);
|
expect(result.llmContent).toMatch(/Successfully overwrote file/);
|
||||||
expect(fs.readFileSync(filePath, 'utf8')).toBe(newContent);
|
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(initialContent);
|
||||||
expect(display.fileDiff).toMatch(newContent);
|
expect(display.fileDiff).toMatch(newContent);
|
||||||
});
|
});
|
||||||
|
|
|
@ -202,7 +202,7 @@ export class WriteFileTool extends BaseTool<WriteFileToolParams, ToolResult> {
|
||||||
? `Successfully created and wrote to new file: ${params.file_path}`
|
? `Successfully created and wrote to new file: ${params.file_path}`
|
||||||
: `Successfully overwrote file: ${params.file_path}`;
|
: `Successfully overwrote file: ${params.file_path}`;
|
||||||
|
|
||||||
const displayResult: FileDiff = { fileDiff };
|
const displayResult: FileDiff = { fileDiff, fileName };
|
||||||
|
|
||||||
return {
|
return {
|
||||||
llmContent: llmSuccessMessage,
|
llmContent: llmSuccessMessage,
|
||||||
|
|
Loading…
Reference in New Issue