abbyswag/qwen-code
1
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2025-12-22 01:37:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

Migrate EditTool, GrepTool, and GlobTool to DeclarativeTool (#5744)

Browse Source
This commit is contained in:
joshualitt
2025-08-07 10:05:37 -07:00
committed by GitHub
parent 0d65baf928
commit 8bac9e7d04
8 changed files with 649 additions and 563 deletions
Download Patch File Download Diff File Expand all files Collapse all files

162
packages/core/src/tools/edit.test.ts
View File

@@ -27,7 +27,7 @@ vi.mock('../utils/editor.js', () => ({
}));
import { describe, it, expect, beforeEach, afterEach, vi, Mock } from 'vitest';
import { EditTool, EditToolParams } from './edit.js';
import { applyReplacement, EditTool, EditToolParams } from './edit.js';
import { FileDiff, ToolConfirmationOutcome } from './tools.js';
import { ToolErrorType } from './tool-error.js';
import path from 'path';
@@ -155,45 +155,30 @@ describe('EditTool', () => {
fs.rmSync(tempDir, { recursive: true, force: true });
});
describe('_applyReplacement', () => {
// Access private method for testing
// Note: `tool` is initialized in `beforeEach` of the parent describe block
describe('applyReplacement', () => {
it('should return newString if isNewFile is true', () => {
expect((tool as any)._applyReplacement(null, 'old', 'new', true)).toBe(
'new',
);
expect(
(tool as any)._applyReplacement('existing', 'old', 'new', true),
).toBe('new');
expect(applyReplacement(null, 'old', 'new', true)).toBe('new');
expect(applyReplacement('existing', 'old', 'new', true)).toBe('new');
});
it('should return newString if currentContent is null and oldString is empty (defensive)', () => {
expect((tool as any)._applyReplacement(null, '', 'new', false)).toBe(
'new',
);
expect(applyReplacement(null, '', 'new', false)).toBe('new');
});
it('should return empty string if currentContent is null and oldString is not empty (defensive)', () => {
expect((tool as any)._applyReplacement(null, 'old', 'new', false)).toBe(
'',
);
expect(applyReplacement(null, 'old', 'new', false)).toBe('');
});
it('should replace oldString with newString in currentContent', () => {
expect(
(tool as any)._applyReplacement(
'hello old world old',
'old',
'new',
false,
),
).toBe('hello new world new');
expect(applyReplacement('hello old world old', 'old', 'new', false)).toBe(
'hello new world new',
);
});
it('should return currentContent if oldString is empty and not a new file', () => {
expect(
(tool as any)._applyReplacement('hello world', '', 'new', false),
).toBe('hello world');
expect(applyReplacement('hello world', '', 'new', false)).toBe(
'hello world',
);
});
});
@@ -239,15 +224,13 @@ describe('EditTool', () => {
filePath = path.join(rootDir, testFile);
});
it('should return false if params are invalid', async () => {
it('should throw an error if params are invalid', async () => {
const params: EditToolParams = {
file_path: 'relative.txt',
old_string: 'old',
new_string: 'new',
};
expect(
await tool.shouldConfirmExecute(params, new AbortController().signal),
).toBe(false);
expect(() => tool.build(params)).toThrow();
});
it('should request confirmation for valid edit', async () => {
@@ -259,8 +242,8 @@ describe('EditTool', () => {
};
// ensureCorrectEdit will be called by shouldConfirmExecute
mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 1 });
const confirmation = await tool.shouldConfirmExecute(
params,
const invocation = tool.build(params);
const confirmation = await invocation.shouldConfirmExecute(
new AbortController().signal,
);
expect(confirmation).toEqual(
@@ -280,9 +263,11 @@ describe('EditTool', () => {
new_string: 'new',
};
mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 0 });
expect(
await tool.shouldConfirmExecute(params, new AbortController().signal),
).toBe(false);
const invocation = tool.build(params);
const confirmation = await invocation.shouldConfirmExecute(
new AbortController().signal,
);
expect(confirmation).toBe(false);
});
it('should return false if multiple occurrences of old_string are found (ensureCorrectEdit returns > 1)', async () => {
@@ -293,9 +278,11 @@ describe('EditTool', () => {
new_string: 'new',
};
mockEnsureCorrectEdit.mockResolvedValueOnce({ params, occurrences: 2 });
expect(
await tool.shouldConfirmExecute(params, new AbortController().signal),
).toBe(false);
const invocation = tool.build(params);
const confirmation = await invocation.shouldConfirmExecute(
new AbortController().signal,
);
expect(confirmation).toBe(false);
});
it('should request confirmation for creating a new file (empty old_string)', async () => {
@@ -310,8 +297,8 @@ describe('EditTool', () => {
// 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,
const invocation = tool.build(params);
const confirmation = await invocation.shouldConfirmExecute(
new AbortController().signal,
);
expect(confirmation).toEqual(
@@ -358,9 +345,8 @@ describe('EditTool', () => {
};
},
);
const confirmation = (await tool.shouldConfirmExecute(
params,
const invocation = tool.build(params);
const confirmation = (await invocation.shouldConfirmExecute(
new AbortController().signal,
)) as FileDiff;
@@ -408,15 +394,13 @@ describe('EditTool', () => {
});
});
it('should return error if params are invalid', async () => {
it('should throw 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/);
expect(() => tool.build(params)).toThrow(/File path must be absolute/);
});
it('should edit an existing file and return diff with fileName', async () => {
@@ -433,12 +417,8 @@ describe('EditTool', () => {
// 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
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(/Successfully modified file/);
expect(fs.readFileSync(filePath, 'utf8')).toBe(newContent);
@@ -461,7 +441,8 @@ describe('EditTool', () => {
(mockConfig.getApprovalMode as Mock).mockReturnValueOnce(
ApprovalMode.AUTO_EDIT,
);
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(/Created new file/);
expect(fs.existsSync(newFilePath)).toBe(true);
@@ -477,7 +458,8 @@ describe('EditTool', () => {
new_string: 'replacement',
};
// The default mockEnsureCorrectEdit will return 0 occurrences for 'nonexistent'
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(
/0 occurrences found for old_string in/,
);
@@ -494,7 +476,8 @@ describe('EditTool', () => {
new_string: 'new',
};
// The default mockEnsureCorrectEdit will return 2 occurrences for 'old'
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(
/Expected 1 occurrence but found 2 for old_string in file/,
);
@@ -512,12 +495,8 @@ describe('EditTool', () => {
expected_replacements: 3,
};
// 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
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(/Successfully modified file/);
expect(fs.readFileSync(filePath, 'utf8')).toBe(
@@ -537,7 +516,8 @@ describe('EditTool', () => {
new_string: 'new',
expected_replacements: 3, // Expecting 3 but only 2 exist
};
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(
/Expected 3 occurrences but found 2 for old_string in file/,
);
@@ -553,7 +533,8 @@ describe('EditTool', () => {
old_string: '',
new_string: 'new content',
};
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(/File already exists, cannot create/);
expect(result.returnDisplay).toMatch(
/Attempted to create a file that already exists/,
@@ -573,7 +554,8 @@ describe('EditTool', () => {
(mockConfig.getApprovalMode as Mock).mockReturnValueOnce(
ApprovalMode.AUTO_EDIT,
);
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(
/User modified the `new_string` content/,
@@ -593,7 +575,8 @@ describe('EditTool', () => {
(mockConfig.getApprovalMode as Mock).mockReturnValueOnce(
ApprovalMode.AUTO_EDIT,
);
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).not.toMatch(
/User modified the `new_string` content/,
@@ -612,7 +595,8 @@ describe('EditTool', () => {
(mockConfig.getApprovalMode as Mock).mockReturnValueOnce(
ApprovalMode.AUTO_EDIT,
);
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).not.toMatch(
/User modified the `new_string` content/,
@@ -627,7 +611,8 @@ describe('EditTool', () => {
old_string: 'identical',
new_string: 'identical',
};
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.llmContent).toMatch(/No changes to apply/);
expect(result.returnDisplay).toMatch(/No changes to apply/);
});
@@ -647,7 +632,8 @@ describe('EditTool', () => {
old_string: 'any',
new_string: 'new',
};
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.error?.type).toBe(ToolErrorType.FILE_NOT_FOUND);
});
@@ -658,7 +644,8 @@ describe('EditTool', () => {
old_string: '',
new_string: 'new content',
};
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.error?.type).toBe(
ToolErrorType.ATTEMPT_TO_CREATE_EXISTING_FILE,
);
@@ -671,7 +658,8 @@ describe('EditTool', () => {
old_string: 'not-found',
new_string: 'new',
};
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_OCCURRENCE_FOUND);
});
@@ -683,7 +671,8 @@ describe('EditTool', () => {
new_string: 'new',
expected_replacements: 3,
};
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.error?.type).toBe(
ToolErrorType.EDIT_EXPECTED_OCCURRENCE_MISMATCH,
);
@@ -696,18 +685,18 @@ describe('EditTool', () => {
old_string: 'content',
new_string: 'content',
};
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_CHANGE);
});
it('should return INVALID_PARAMETERS error for relative path', async () => {
it('should throw INVALID_PARAMETERS error for relative path', async () => {
const params: EditToolParams = {
file_path: 'relative/path.txt',
old_string: 'a',
new_string: 'b',
};
const result = await tool.execute(params, new AbortController().signal);
expect(result.error?.type).toBe(ToolErrorType.INVALID_TOOL_PARAMS);
expect(() => tool.build(params)).toThrow();
});
it('should return FILE_WRITE_FAILURE on write error', async () => {
@@ -720,7 +709,8 @@ describe('EditTool', () => {
old_string: 'content',
new_string: 'new content',
};
const result = await tool.execute(params, new AbortController().signal);
const invocation = tool.build(params);
const result = await invocation.execute(new AbortController().signal);
expect(result.error?.type).toBe(ToolErrorType.FILE_WRITE_FAILURE);
});
});
@@ -733,8 +723,9 @@ describe('EditTool', () => {
old_string: 'identical_string',
new_string: 'identical_string',
};
const invocation = tool.build(params);
// shortenPath will be called internally, resulting in just the file name
expect(tool.getDescription(params)).toBe(
expect(invocation.getDescription()).toBe(
`No file changes to ${testFileName}`,
);
});
@@ -746,9 +737,10 @@ describe('EditTool', () => {
old_string: 'this is the old string value',
new_string: 'this is the new string value',
};
const invocation = tool.build(params);
// shortenPath will be called internally, resulting in just the file name
// The snippets are truncated at 30 chars + '...'
expect(tool.getDescription(params)).toBe(
expect(invocation.getDescription()).toBe(
`${testFileName}: this is the old string value => this is the new string value`,
);
});
@@ -760,7 +752,8 @@ describe('EditTool', () => {
old_string: 'old',
new_string: 'new',
};
expect(tool.getDescription(params)).toBe(`${testFileName}: old => new`);
const invocation = tool.build(params);
expect(invocation.getDescription()).toBe(`${testFileName}: old => new`);
});
it('should truncate long strings in the description', () => {
@@ -772,7 +765,8 @@ describe('EditTool', () => {
new_string:
'this is a very long new string that will also be truncated',
};
expect(tool.getDescription(params)).toBe(
const invocation = tool.build(params);
expect(invocation.getDescription()).toBe(
`${testFileName}: this is a very long old string... => this is a very long new string...`,
);
});
@@ -839,8 +833,8 @@ describe('EditTool', () => {
content: modifiedContent,
});
const confirmation = await tool.shouldConfirmExecute(
params,
const invocation = tool.build(params);
const confirmation = await invocation.shouldConfirmExecute(
new AbortController().signal,
);