diff --git a/.agents/types/tools.ts b/.agents/types/tools.ts index 754e54d78a..15d0363901 100644 --- a/.agents/types/tools.ts +++ b/.agents/types/tools.ts @@ -181,10 +181,10 @@ export interface ProposeStrReplaceParams { /** Array of replacements to make. */ replacements: { /** The string to replace. This must be an *exact match* of the string you want to replace, including whitespace and punctuation. */ - old: string - /** The string to replace the corresponding old string with. Can be empty to delete. */ - new: string - /** Whether to allow multiple replacements of old string. */ + oldString: string + /** The string to replace the corresponding oldString with. Can be empty to delete. */ + newString: string + /** Whether to allow multiple replacements of oldString. */ allowMultiple?: boolean }[] } @@ -305,10 +305,10 @@ export interface StrReplaceParams { /** Array of replacements to make. */ replacements: { /** The string to replace. This must be an *exact match* of the string you want to replace, including whitespace and punctuation. */ - old: string - /** The string to replace the corresponding old string with. Can be empty to delete. */ - new: string - /** Whether to allow multiple replacements of old string. */ + oldString: string + /** The string to replace the corresponding oldString with. Can be empty to delete. */ + newString: string + /** Whether to allow multiple replacements of oldString. */ allowMultiple?: boolean }[] } diff --git a/agents-graveyard/editor/reviewer-editor.ts b/agents-graveyard/editor/reviewer-editor.ts index c6cfe42b6a..f76d8d559d 100644 --- a/agents-graveyard/editor/reviewer-editor.ts +++ b/agents-graveyard/editor/reviewer-editor.ts @@ -36,12 +36,12 @@ Write out what changes you would make using the tool call format below. Use this "path": "path/to/file", "replacements": [ { - "old": "exact old code", - "new": "exact new code" + "oldString": "exact old code", + "newString": "exact new code" }, { - "old": "exact old code 2", - "new": "exact new code 2" + "oldString": "exact old code 2", + "newString": "exact new code 2" }, ] } diff --git a/agents/editor/best-of-n/editor-implementor.ts b/agents/editor/best-of-n/editor-implementor.ts index fe9fe13ebf..2afc66d68e 100644 --- a/agents/editor/best-of-n/editor-implementor.ts +++ b/agents/editor/best-of-n/editor-implementor.ts @@ -51,12 +51,12 @@ You can make multiple tool calls across multiple steps to complete the implement "path": "path/to/file", "replacements": [ { - "old": "exact old code", - "new": "exact new code" + "oldString": "exact old code", + "newString": "exact new code" }, { - "old": "exact old code 2", - "new": "exact new code 2" + "oldString": "exact old code 2", + "newString": "exact new code 2" }, ] } @@ -72,9 +72,10 @@ OR for new files or major rewrites: "content": "Complete file content" } -${isGpt5 || isGemini - ? `` - : ` +${ + isGpt5 || isGemini + ? `` + : ` IMPORTANT: Before you start writing your implementation, you should use tags to think about the best way to implement the changes. You should think really really hard to make sure you implement the changes in the best way possible. Take as much time as you to think through all the cases to produce the best changes. You can also use tags interspersed between tool calls to think about the best way to implement the changes. @@ -102,7 +103,7 @@ You can also use tags interspersed between tool calls to think about the ` - } +} After the edit tool calls, you can optionally mention any follow-up steps to take, like deleting a file, or a specific way to validate the changes. There's no need to use the set_output tool as your entire response will be included in the output. diff --git a/agents/editor/editor.ts b/agents/editor/editor.ts index 443724f67d..a0cac064c6 100644 --- a/agents/editor/editor.ts +++ b/agents/editor/editor.ts @@ -61,12 +61,12 @@ Write out what changes you would make using the tool call format below. Use this "path": "path/to/file", "replacements": [ { - "old": "exact old code", - "new": "exact new code" + "oldString": "exact old code", + "newString": "exact new code" }, { - "old": "exact old code 2", - "new": "exact new code 2" + "oldString": "exact old code 2", + "newString": "exact new code 2" }, ] } diff --git a/agents/types/tools.ts b/agents/types/tools.ts index 9cfe1cdf2e..cb3882fc04 100644 --- a/agents/types/tools.ts +++ b/agents/types/tools.ts @@ -226,10 +226,10 @@ export interface ProposeStrReplaceParams { /** Array of replacements to make. */ replacements: { /** The string to replace. This must be an *exact match* of the string you want to replace, including whitespace and punctuation. */ - old: string - /** The string to replace the corresponding old string with. Can be empty to delete. */ - new: string - /** Whether to allow multiple replacements of old string. */ + oldString: string + /** The string to replace the corresponding oldString with. Can be empty to delete. */ + newString: string + /** Whether to allow multiple replacements of oldString. */ allowMultiple?: boolean }[] } @@ -358,10 +358,10 @@ export interface StrReplaceParams { /** Array of replacements to make. */ replacements: { /** The string to replace. This must be an *exact match* of the string you want to replace, including whitespace and punctuation. */ - old: string - /** The string to replace the corresponding old string with. Can be empty to delete. */ - new: string - /** Whether to allow multiple replacements of old string. */ + oldString: string + /** The string to replace the corresponding oldString with. Can be empty to delete. */ + newString: string + /** Whether to allow multiple replacements of oldString. */ allowMultiple?: boolean }[] } diff --git a/cli/src/components/tools/str-replace.tsx b/cli/src/components/tools/str-replace.tsx index 881152472e..10e00672cf 100644 --- a/cli/src/components/tools/str-replace.tsx +++ b/cli/src/components/tools/str-replace.tsx @@ -3,43 +3,14 @@ import { TextAttributes } from '@opentui/core' import { DiffViewer } from './diff-viewer' import { defineToolComponent } from './types' import { useTheme } from '../../hooks/use-theme' +import { + extractDiff, + extractFilePath, + isCreateFile, +} from '../../utils/implementor-helpers' import type { ToolRenderConfig } from './types' -function extractValueForKey(output: string, key: string): string | null { - if (!output) return null - const lines = output.split('\n') - for (let i = 0; i < lines.length; i++) { - const line = lines[i] - const match = line.match(/^\s*([A-Za-z0-9_]+):\s*(.*)$/) - if (match && match[1] === key) { - const rest = match[2] - if (rest.trim().startsWith('|')) { - const baseIndent = lines[i + 1]?.match(/^\s*/)?.[0].length ?? 0 - const acc: string[] = [] - for (let j = i + 1; j < lines.length; j++) { - const l = lines[j] - const indent = l.match(/^\s*/)?.[0].length ?? 0 - if (l.trim().length === 0) { - acc.push('') - continue - } - if (indent < baseIndent) break - acc.push(l.slice(baseIndent)) - } - return acc.join('\n') - } else { - let val = rest.trim() - if (val.startsWith('"') && val.endsWith('"')) { - val = val.slice(1, -1) - } - return val - } - } - } - return null -} - interface EditHeaderProps { name: string filePath: string | null @@ -73,7 +44,7 @@ const EditBody = ({ name, filePath, diffText, isCreate }: EditBodyProps) => { return ( - {!isCreate && ( + {!isCreate && diffText.length > 0 && ( @@ -86,18 +57,9 @@ export const StrReplaceComponent = defineToolComponent({ toolName: 'str_replace', render(toolBlock): ToolRenderConfig { - const outputStr = - typeof toolBlock.output === 'string' ? toolBlock.output : '' - const diff = - extractValueForKey(outputStr, 'unifiedDiff') || - extractValueForKey(outputStr, 'patch') - const filePath = - extractValueForKey(outputStr, 'file') || - (typeof (toolBlock.input as any)?.path === 'string' - ? (toolBlock.input as any).path - : null) - const message = extractValueForKey(outputStr, 'message') - const isCreate = message === 'Created new file' + const diff = extractDiff(toolBlock) + const filePath = extractFilePath(toolBlock) + const isCreate = isCreateFile(toolBlock) return { content: ( diff --git a/cli/src/utils/__tests__/implementor-helpers.test.ts b/cli/src/utils/__tests__/implementor-helpers.test.ts index 83bcf2490f..03699fc41c 100644 --- a/cli/src/utils/__tests__/implementor-helpers.test.ts +++ b/cli/src/utils/__tests__/implementor-helpers.test.ts @@ -19,7 +19,12 @@ import { getMultiPromptPreview, } from '../implementor-helpers' -import type { ToolContentBlock, ContentBlock, AgentContentBlock, TextContentBlock } from '../../types/chat' +import type { + ToolContentBlock, + ContentBlock, + AgentContentBlock, + TextContentBlock, +} from '../../types/chat' describe('extractValueForKey', () => { test('extracts simple key-value pairs', () => { @@ -104,9 +109,7 @@ describe('extractDiff', () => { toolCallId: 'test-1', toolName: 'str_replace', input: { - replacements: [ - { old: 'const x = 1', new: 'const x = 2' } - ] + replacements: [{ oldString: 'const x = 1', newString: 'const x = 2' }], }, } const diff = extractDiff(block) @@ -114,6 +117,82 @@ describe('extractDiff', () => { expect(diff).toContain('+ const x = 2') }) + test('constructs diff from successful str_replace input when output omits diff', () => { + const block: ToolContentBlock = { + type: 'tool', + toolCallId: 'test-1', + toolName: 'str_replace', + input: { + replacements: [{ oldString: 'const x = 1', newString: 'const x = 2' }], + }, + output: 'message: String replace applied successfully.', + } + const diff = extractDiff(block) + expect(diff).toContain('- const x = 1') + expect(diff).toContain('+ const x = 2') + }) + + test('constructs diff from successful str_replace input with warning output', () => { + const block: ToolContentBlock = { + type: 'tool', + toolCallId: 'test-1', + toolName: 'str_replace', + input: { + replacements: [{ oldString: 'const x = 1', newString: 'const x = 2' }], + }, + output: `message: | + Matched with indentation modification + + String replace applied successfully.`, + } + const diff = extractDiff(block) + expect(diff).toContain('- const x = 1') + expect(diff).toContain('+ const x = 2') + }) + + test('uses patch content from successful str_replace input when output omits diff', () => { + const block: ToolContentBlock = { + type: 'tool', + toolCallId: 'test-1', + toolName: 'str_replace', + input: { type: 'patch', content: '- const x = 1\n+ const x = 2' }, + output: 'message: String replace applied successfully.', + } + expect(extractDiff(block)).toBe('- const x = 1\n+ const x = 2') + }) + + test('returns null for failed str_replace output without a diff', () => { + const block: ToolContentBlock = { + type: 'tool', + toolCallId: 'test-1', + toolName: 'str_replace', + input: { + replacements: [{ oldString: 'const x = 1', newString: 'const x = 2' }], + }, + output: 'No change to the file', + } + expect(extractDiff(block)).toBeNull() + }) + + test('returns null for failed str_replace output even when it includes patch input', () => { + const block: ToolContentBlock = { + type: 'tool', + toolCallId: 'test-1', + toolName: 'str_replace', + input: { type: 'patch', content: '- const x = 1\n+ const x = 2' }, + outputRaw: [ + { + type: 'json', + value: { + errorMessage: 'Failed to apply patch.', + patch: '- const x = 1\n+ const x = 2', + }, + }, + ], + } + expect(extractDiff(block)).toBeNull() + }) + test('constructs diff from write_file input', () => { const block: ToolContentBlock = { type: 'tool', @@ -125,15 +204,36 @@ describe('extractDiff', () => { expect(diff).toBe('+ line1\n+ line2') }) + test('constructs diff from successful write_file input when output omits diff', () => { + const block: ToolContentBlock = { + type: 'tool', + toolCallId: 'test-1', + toolName: 'write_file', + input: { content: 'line1\nline2' }, + output: 'message: Overwrote file successfully.', + } + const diff = extractDiff(block) + expect(diff).toBe('+ line1\n+ line2') + }) + + test('returns null for failed write_file output without a diff', () => { + const block: ToolContentBlock = { + type: 'tool', + toolCallId: 'test-1', + toolName: 'write_file', + input: { content: 'line1\nline2' }, + output: 'Failed to write to file', + } + expect(extractDiff(block)).toBeNull() + }) + test('constructs diff from propose_str_replace input', () => { const block: ToolContentBlock = { type: 'tool', toolCallId: 'test-1', toolName: 'propose_str_replace', input: { - replacements: [ - { old: 'const x = 1', new: 'const x = 2' } - ] + replacements: [{ oldString: 'const x = 1', newString: 'const x = 2' }], }, } const diff = extractDiff(block) @@ -178,8 +278,16 @@ describe('parseDiffStats', () => { }) test('handles empty diff', () => { - expect(parseDiffStats(undefined)).toEqual({ linesAdded: 0, linesRemoved: 0, hunks: 0 }) - expect(parseDiffStats('')).toEqual({ linesAdded: 0, linesRemoved: 0, hunks: 0 }) + expect(parseDiffStats(undefined)).toEqual({ + linesAdded: 0, + linesRemoved: 0, + hunks: 0, + }) + expect(parseDiffStats('')).toEqual({ + linesAdded: 0, + linesRemoved: 0, + hunks: 0, + }) }) test('ignores +++ and --- headers', () => { @@ -206,6 +314,17 @@ describe('getFileChangeType', () => { expect(getFileChangeType(block)).toBe('A') }) + test('returns A for successful file creation', () => { + const block: ToolContentBlock = { + type: 'tool', + toolCallId: 'test-1', + toolName: 'write_file', + input: {}, + output: 'message: Created file successfully.', + } + expect(getFileChangeType(block)).toBe('A') + }) + test('returns M for write_file modification', () => { const block: ToolContentBlock = { type: 'tool', @@ -264,7 +383,9 @@ describe('getFileStatsFromBlocks', () => { toolCallId: 'test-2', toolName: 'str_replace', input: { path: 'file.ts' }, - outputRaw: [{ type: 'json', value: { unifiedDiff: '+line3\n-removed' } }], + outputRaw: [ + { type: 'json', value: { unifiedDiff: '+line3\n-removed' } }, + ], }, ] const stats = getFileStatsFromBlocks(blocks) @@ -307,6 +428,25 @@ describe('getFileStatsFromBlocks', () => { const stats = getFileStatsFromBlocks(blocks) expect(stats).toHaveLength(0) }) + + test('ignores failed edit tools', () => { + const blocks: ContentBlock[] = [ + { + type: 'tool', + toolCallId: 'test-1', + toolName: 'str_replace', + input: { + path: 'file.ts', + replacements: [ + { oldString: 'const x = 1', newString: 'const x = 2' }, + ], + }, + output: 'No change to the file', + }, + ] + const stats = getFileStatsFromBlocks(blocks) + expect(stats).toHaveLength(0) + }) }) describe('buildActivityTimeline', () => { @@ -354,20 +494,53 @@ describe('buildActivityTimeline', () => { expect(timeline).toHaveLength(1) expect(timeline[0].content).toBe('Normal text') }) + + test('skips failed edit tools', () => { + const blocks: ContentBlock[] = [ + { + type: 'text', + content: 'Trying an edit', + } as TextContentBlock, + { + type: 'tool', + toolCallId: 'test-1', + toolName: 'write_file', + input: { path: 'file.ts', content: 'new content' }, + output: 'Failed to write to file', + }, + ] + const timeline = buildActivityTimeline(blocks) + expect(timeline).toHaveLength(1) + expect(timeline[0].type).toBe('commentary') + }) }) describe('isImplementorAgent', () => { test('identifies implementor agents', () => { - expect(isImplementorAgent({ agentType: 'editor-implementor', blocks: [] })).toBe(true) - expect(isImplementorAgent({ agentType: 'editor-implementor-opus', blocks: [] })).toBe(true) - expect(isImplementorAgent({ agentType: 'editor-implementor-gpt-5', blocks: [] })).toBe(true) - expect(isImplementorAgent({ agentType: 'editor-implementor2', blocks: [] })).toBe(true) + expect( + isImplementorAgent({ agentType: 'editor-implementor', blocks: [] }), + ).toBe(true) + expect( + isImplementorAgent({ agentType: 'editor-implementor-opus', blocks: [] }), + ).toBe(true) + expect( + isImplementorAgent({ agentType: 'editor-implementor-gpt-5', blocks: [] }), + ).toBe(true) + expect( + isImplementorAgent({ agentType: 'editor-implementor2', blocks: [] }), + ).toBe(true) }) test('rejects non-implementor agents', () => { - expect(isImplementorAgent({ agentType: 'file-picker', blocks: [] })).toBe(false) - expect(isImplementorAgent({ agentType: 'commander', blocks: [] })).toBe(false) - expect(isImplementorAgent({ agentType: 'best-of-n-selector', blocks: [] })).toBe(false) + expect(isImplementorAgent({ agentType: 'file-picker', blocks: [] })).toBe( + false, + ) + expect(isImplementorAgent({ agentType: 'commander', blocks: [] })).toBe( + false, + ) + expect( + isImplementorAgent({ agentType: 'best-of-n-selector', blocks: [] }), + ).toBe(false) }) }) @@ -376,20 +549,48 @@ describe('getImplementorDisplayName', () => { expect(getImplementorDisplayName('editor-implementor')).toBe('Sonnet') expect(getImplementorDisplayName('editor-implementor-opus')).toBe('Opus') expect(getImplementorDisplayName('editor-implementor-gpt-5')).toBe('GPT-5') - expect(getImplementorDisplayName('editor-implementor-gemini')).toBe('Gemini') + expect(getImplementorDisplayName('editor-implementor-gemini')).toBe( + 'Gemini', + ) }) test('adds index when provided', () => { expect(getImplementorDisplayName('editor-implementor', 0)).toBe('Sonnet #1') - expect(getImplementorDisplayName('editor-implementor-opus', 2)).toBe('Opus #3') + expect(getImplementorDisplayName('editor-implementor-opus', 2)).toBe( + 'Opus #3', + ) }) }) describe('getImplementorIndex', () => { test('returns index among same-type siblings', () => { - const agent1 = { type: 'agent', agentId: 'a1', agentName: 'Impl 1', agentType: 'editor-implementor', content: '', status: 'complete', blocks: [] } as AgentContentBlock - const agent2 = { type: 'agent', agentId: 'a2', agentName: 'Impl 2', agentType: 'editor-implementor', content: '', status: 'complete', blocks: [] } as AgentContentBlock - const agent3 = { type: 'agent', agentId: 'a3', agentName: 'Impl 3', agentType: 'editor-implementor-opus', content: '', status: 'complete', blocks: [] } as AgentContentBlock + const agent1 = { + type: 'agent', + agentId: 'a1', + agentName: 'Impl 1', + agentType: 'editor-implementor', + content: '', + status: 'complete', + blocks: [], + } as AgentContentBlock + const agent2 = { + type: 'agent', + agentId: 'a2', + agentName: 'Impl 2', + agentType: 'editor-implementor', + content: '', + status: 'complete', + blocks: [], + } as AgentContentBlock + const agent3 = { + type: 'agent', + agentId: 'a3', + agentName: 'Impl 3', + agentType: 'editor-implementor-opus', + content: '', + status: 'complete', + blocks: [], + } as AgentContentBlock const siblings: ContentBlock[] = [agent1, agent2, agent3] expect(getImplementorIndex(agent1, siblings)).toBe(0) @@ -398,7 +599,15 @@ describe('getImplementorIndex', () => { }) test('returns undefined for non-implementor', () => { - const filePicker = { type: 'agent', agentId: 'fp1', agentName: 'File Picker', agentType: 'file-picker', content: '', status: 'complete', blocks: [] } as AgentContentBlock + const filePicker = { + type: 'agent', + agentId: 'fp1', + agentName: 'File Picker', + agentType: 'file-picker', + content: '', + status: 'complete', + blocks: [], + } as AgentContentBlock const siblings: ContentBlock[] = [filePicker] expect(getImplementorIndex(filePicker, siblings)).toBeUndefined() @@ -406,10 +615,11 @@ describe('getImplementorIndex', () => { }) describe('groupConsecutiveBlocks', () => { - const createTextBlock = (content: string): TextContentBlock => ({ - type: 'text', - content, - } as TextContentBlock) + const createTextBlock = (content: string): TextContentBlock => + ({ + type: 'text', + content, + }) as TextContentBlock const createToolBlock = (toolName: string): ToolContentBlock => ({ type: 'tool', @@ -418,15 +628,19 @@ describe('groupConsecutiveBlocks', () => { input: {}, }) - const createAgentBlock = (agentType: string, agentId: string): AgentContentBlock => ({ - type: 'agent', - agentId, - agentName: agentType, - agentType, - content: '', - status: 'complete', - blocks: [], - } as AgentContentBlock) + const createAgentBlock = ( + agentType: string, + agentId: string, + ): AgentContentBlock => + ({ + type: 'agent', + agentId, + agentName: agentType, + agentType, + content: '', + status: 'complete', + blocks: [], + }) as AgentContentBlock test('groups consecutive matching blocks from start', () => { const blocks: ContentBlock[] = [ @@ -530,7 +744,8 @@ describe('groupConsecutiveBlocks', () => { createTextBlock('done'), ] const isEditTool = (b: ContentBlock): b is ToolContentBlock => - b.type === 'tool' && ['str_replace', 'write_file'].includes(b.toolName as string) + b.type === 'tool' && + ['str_replace', 'write_file'].includes(b.toolName as string) const result = groupConsecutiveBlocks(blocks, 0, isEditTool) expect(result.group).toHaveLength(2) @@ -541,30 +756,39 @@ describe('groupConsecutiveBlocks', () => { }) describe('groupConsecutiveImplementors', () => { - const createImplementorAgent = (id: string, agentType = 'editor-implementor'): AgentContentBlock => ({ - type: 'agent', - agentId: id, - agentName: 'Implementor', - agentType, - content: '', - status: 'complete', - blocks: [], - } as AgentContentBlock) - - const createNonImplementorAgent = (id: string, agentType: string): AgentContentBlock => ({ - type: 'agent', - agentId: id, - agentName: agentType, - agentType, - content: '', - status: 'complete', - blocks: [], - } as AgentContentBlock) - - const createTextBlock = (content: string): TextContentBlock => ({ - type: 'text', - content, - } as TextContentBlock) + const createImplementorAgent = ( + id: string, + agentType = 'editor-implementor', + ): AgentContentBlock => + ({ + type: 'agent', + agentId: id, + agentName: 'Implementor', + agentType, + content: '', + status: 'complete', + blocks: [], + }) as AgentContentBlock + + const createNonImplementorAgent = ( + id: string, + agentType: string, + ): AgentContentBlock => + ({ + type: 'agent', + agentId: id, + agentName: agentType, + agentType, + content: '', + status: 'complete', + blocks: [], + }) as AgentContentBlock + + const createTextBlock = (content: string): TextContentBlock => + ({ + type: 'text', + content, + }) as TextContentBlock test('groups consecutive implementor agents', () => { const blocks: ContentBlock[] = [ @@ -654,30 +878,36 @@ describe('groupConsecutiveImplementors', () => { }) describe('groupConsecutiveNonImplementorAgents', () => { - const createImplementorAgent = (id: string): AgentContentBlock => ({ - type: 'agent', - agentId: id, - agentName: 'Implementor', - agentType: 'editor-implementor', - content: '', - status: 'complete', - blocks: [], - } as AgentContentBlock) - - const createNonImplementorAgent = (id: string, agentType: string): AgentContentBlock => ({ - type: 'agent', - agentId: id, - agentName: agentType, - agentType, - content: '', - status: 'complete', - blocks: [], - } as AgentContentBlock) - - const createTextBlock = (content: string): TextContentBlock => ({ - type: 'text', - content, - } as TextContentBlock) + const createImplementorAgent = (id: string): AgentContentBlock => + ({ + type: 'agent', + agentId: id, + agentName: 'Implementor', + agentType: 'editor-implementor', + content: '', + status: 'complete', + blocks: [], + }) as AgentContentBlock + + const createNonImplementorAgent = ( + id: string, + agentType: string, + ): AgentContentBlock => + ({ + type: 'agent', + agentId: id, + agentName: agentType, + agentType, + content: '', + status: 'complete', + blocks: [], + }) as AgentContentBlock + + const createTextBlock = (content: string): TextContentBlock => + ({ + type: 'text', + content, + }) as TextContentBlock test('groups consecutive non-implementor agents', () => { const blocks: ContentBlock[] = [ @@ -776,25 +1006,32 @@ describe('groupConsecutiveNonImplementorAgents', () => { }) describe('getMultiPromptProgress', () => { - const createImplementorAgent = (id: string, status: 'running' | 'complete' | 'failed' | 'cancelled' = 'complete'): AgentContentBlock => ({ - type: 'agent', - agentId: id, - agentName: 'Implementor', - agentType: 'editor-implementor-opus', - content: '', - status, - blocks: [], - } as AgentContentBlock) - - const createSelectorAgent = (status: 'running' | 'complete' = 'running'): AgentContentBlock => ({ - type: 'agent', - agentId: 'selector-1', - agentName: 'Selector', - agentType: 'best-of-n-selector2', - content: '', - status, - blocks: [], - } as AgentContentBlock) + const createImplementorAgent = ( + id: string, + status: 'running' | 'complete' | 'failed' | 'cancelled' = 'complete', + ): AgentContentBlock => + ({ + type: 'agent', + agentId: id, + agentName: 'Implementor', + agentType: 'editor-implementor-opus', + content: '', + status, + blocks: [], + }) as AgentContentBlock + + const createSelectorAgent = ( + status: 'running' | 'complete' = 'running', + ): AgentContentBlock => + ({ + type: 'agent', + agentId: 'selector-1', + agentName: 'Selector', + agentType: 'best-of-n-selector2', + content: '', + status, + blocks: [], + }) as AgentContentBlock test('returns null for empty blocks', () => { expect(getMultiPromptProgress([])).toBeNull() @@ -877,31 +1114,40 @@ describe('getMultiPromptProgress', () => { }) describe('getMultiPromptPreview', () => { - const createImplementorAgent = (id: string, status: 'running' | 'complete' | 'failed' | 'cancelled' = 'complete'): AgentContentBlock => ({ - type: 'agent', - agentId: id, - agentName: 'Implementor', - agentType: 'editor-implementor-opus', - content: '', - status, - blocks: [], - } as AgentContentBlock) - - const createSelectorAgent = (status: 'running' | 'complete' = 'running'): AgentContentBlock => ({ - type: 'agent', - agentId: 'selector-1', - agentName: 'Selector', - agentType: 'best-of-n-selector2', - content: '', - status, - blocks: [], - } as AgentContentBlock) + const createImplementorAgent = ( + id: string, + status: 'running' | 'complete' | 'failed' | 'cancelled' = 'complete', + ): AgentContentBlock => + ({ + type: 'agent', + agentId: id, + agentName: 'Implementor', + agentType: 'editor-implementor-opus', + content: '', + status, + blocks: [], + }) as AgentContentBlock + + const createSelectorAgent = ( + status: 'running' | 'complete' = 'running', + ): AgentContentBlock => + ({ + type: 'agent', + agentId: 'selector-1', + agentName: 'Selector', + agentType: 'best-of-n-selector2', + content: '', + status, + blocks: [], + }) as AgentContentBlock const createSetOutputBlock = (reason?: string): ToolContentBlock => ({ type: 'tool', toolCallId: 'set-output-1', toolName: 'set_output', - input: reason ? { data: { chosenStrategy: 'strategy A', reason } } : { data: { chosenStrategy: 'strategy A' } }, + input: reason + ? { data: { chosenStrategy: 'strategy A', reason } } + : { data: { chosenStrategy: 'strategy A' } }, }) test('returns null for empty blocks', () => { @@ -934,7 +1180,9 @@ describe('getMultiPromptPreview', () => { createImplementorAgent('impl-3', 'complete'), createSelectorAgent('running'), ] - expect(getMultiPromptPreview(blocks)).toBe('3 proposals complete • Selecting best...') + expect(getMultiPromptPreview(blocks)).toBe( + '3 proposals complete • Selecting best...', + ) }) test('shows applying message when selector is complete but agent not done', () => { @@ -943,7 +1191,9 @@ describe('getMultiPromptPreview', () => { createImplementorAgent('impl-2', 'complete'), createSelectorAgent('complete'), ] - expect(getMultiPromptPreview(blocks, false)).toBe('Applying selected changes...') + expect(getMultiPromptPreview(blocks, false)).toBe( + 'Applying selected changes...', + ) }) test('shows evaluation count when agent is complete without reason', () => { @@ -962,7 +1212,9 @@ describe('getMultiPromptPreview', () => { createSetOutputBlock('best implementation with proper error handling'), ] const preview = getMultiPromptPreview(blocks, true) - expect(preview).toBe('2 proposals evaluated\nBest implementation with proper error handling') + expect(preview).toBe( + '2 proposals evaluated\nBest implementation with proper error handling', + ) }) test('capitalizes first letter of reason', () => { @@ -989,7 +1241,9 @@ describe('getMultiPromptPreview', () => { createImplementorAgent('impl-2', 'complete'), createImplementorAgent('impl-3', 'failed'), ] - expect(getMultiPromptPreview(blocks)).toBe('2/3 proposals complete (1 failed)') + expect(getMultiPromptPreview(blocks)).toBe( + '2/3 proposals complete (1 failed)', + ) }) test('treats failed implementors as finished for progress', () => { @@ -999,7 +1253,9 @@ describe('getMultiPromptPreview', () => { createImplementorAgent('impl-3', 'complete'), ] // All 3 are finished (1 complete + 2 failed/cancelled), so should show completion message - expect(getMultiPromptPreview(blocks)).toBe('1/3 proposals complete (2 failed)') + expect(getMultiPromptPreview(blocks)).toBe( + '1/3 proposals complete (2 failed)', + ) }) }) @@ -1011,20 +1267,22 @@ describe('groupConsecutiveToolBlocks', () => { input: {}, }) - const createTextBlock = (content: string): TextContentBlock => ({ - type: 'text', - content, - } as TextContentBlock) - - const createAgentBlock = (id: string): AgentContentBlock => ({ - type: 'agent', - agentId: id, - agentName: 'Test Agent', - agentType: 'file-picker', - content: '', - status: 'complete', - blocks: [], - } as AgentContentBlock) + const createTextBlock = (content: string): TextContentBlock => + ({ + type: 'text', + content, + }) as TextContentBlock + + const createAgentBlock = (id: string): AgentContentBlock => + ({ + type: 'agent', + agentId: id, + agentName: 'Test Agent', + agentType: 'file-picker', + content: '', + status: 'complete', + blocks: [], + }) as AgentContentBlock test('groups consecutive tool blocks', () => { const blocks: ContentBlock[] = [ diff --git a/cli/src/utils/implementor-helpers.ts b/cli/src/utils/implementor-helpers.ts index ca757ba52e..3fb5027a3f 100644 --- a/cli/src/utils/implementor-helpers.ts +++ b/cli/src/utils/implementor-helpers.ts @@ -25,6 +25,18 @@ const isProposedToolName = (toolName: ToolContentBlock['toolName']): boolean => const getBaseToolName = (toolName: ToolContentBlock['toolName']): string => isProposedToolName(toolName) ? toolName.slice('propose_'.length) : toolName +const SUCCESSFUL_EDIT_MESSAGES = [ + 'String replace applied successfully', + 'Created file successfully', + 'Created new file', + 'Overwrote file successfully', + 'Wrote file successfully', + 'Updated file', + 'Proposed new file', + 'Proposed changes', + 'Proposed string replacement', +] as const + const hasProposedTools = (blocks?: ContentBlock[]): boolean => { if (!blocks || blocks.length === 0) return false @@ -221,38 +233,61 @@ export function extractFilePath(toolBlock: ToolContentBlock): string | null { * For proposed tools (implementors): construct diff from input replacements. */ export function extractDiff(toolBlock: ToolContentBlock): string | null { + let hasSuccessfulOutput = false + // First try to get from outputRaw (for executed tool results) // outputRaw is typically an array like [{type: "json", value: {unifiedDiff: "..."}}] const outputRaw = toolBlock.outputRaw as unknown if (Array.isArray(outputRaw) && outputRaw[0]?.value) { const value = outputRaw[0].value as Record + if (hasErrorMessage(value)) return null + if (isSuccessfulEditMessage(value.message)) hasSuccessfulOutput = true if (value.unifiedDiff) return value.unifiedDiff as string if (value.patch) return value.patch as string } // Also check direct properties (in case format differs) if (typeof outputRaw === 'object' && outputRaw !== null) { const rawObj = outputRaw as Record + if (hasErrorMessage(rawObj)) return null + if (isSuccessfulEditMessage(rawObj.message)) hasSuccessfulOutput = true if (rawObj.unifiedDiff) return rawObj.unifiedDiff as string if (rawObj.patch) return rawObj.patch as string } // Try to get from output string (key: value format) const outputStr = typeof toolBlock.output === 'string' ? toolBlock.output : '' + const message = extractValueForKey(outputStr, 'message') const diffFromOutput = extractValueForKey(outputStr, 'unifiedDiff') || extractValueForKey(outputStr, 'patch') + if (hasFailedEditOutput({ outputStr, message, diffFromOutput })) { + return null + } + if (isSuccessfulEditMessage(message)) { + hasSuccessfulOutput = true + } + if (diffFromOutput) { return diffFromOutput } - // For proposed edits (no output yet): construct diff from input + // For proposed/pending edits, or confirmed successful executions, construct + // the preview from input when the result omits a diff. + const canUseInputFallback = + isProposedToolName(toolBlock.toolName) || + outputStr === '' || + hasSuccessfulOutput + if (!canUseInputFallback) { + return null + } + const input = toolBlock.input as Record const baseToolName = getBaseToolName(toolBlock.toolName) // Handle str_replace: construct diff from replacements if (baseToolName === 'str_replace' && Array.isArray(input?.replacements)) { - const replacements = input.replacements as { old: string; new: string }[] + const replacements = input.replacements as ReplacementInput[] if (replacements.length > 0) { return constructDiffFromReplacements(replacements) } @@ -271,22 +306,96 @@ export function extractDiff(toolBlock: ToolContentBlock): string | null { return null } +function hasErrorMessage(value: Record): boolean { + return Boolean(value.errorMessage || (value.value as any)?.errorMessage) +} + +function hasFailedEditOutput(params: { + outputStr: string + message: string | null + diffFromOutput: string | null +}): boolean { + const { outputStr, message, diffFromOutput } = params + const trimmedOutput = outputStr.trim() + if (!trimmedOutput) { + return false + } + if ( + extractValueForKey(outputStr, 'errorMessage') || + isErrorOutput(outputStr) + ) { + return true + } + if (diffFromOutput || isSuccessfulEditMessage(message)) { + return false + } + return !isSuccessfulEditMessage(trimmedOutput) +} + +function isFailedEditToolBlock(toolBlock: ToolContentBlock): boolean { + const outputRaw = toolBlock.outputRaw as unknown + if (Array.isArray(outputRaw) && outputRaw[0]?.value) { + const value = outputRaw[0].value as Record + if (hasErrorMessage(value)) return true + } + if (typeof outputRaw === 'object' && outputRaw !== null) { + const rawObj = outputRaw as Record + if (hasErrorMessage(rawObj)) return true + } + + const outputStr = typeof toolBlock.output === 'string' ? toolBlock.output : '' + const message = extractValueForKey(outputStr, 'message') + const diffFromOutput = + extractValueForKey(outputStr, 'unifiedDiff') || + extractValueForKey(outputStr, 'patch') + return hasFailedEditOutput({ outputStr, message, diffFromOutput }) +} + +function isSuccessfulEditMessage(message: unknown): boolean { + if (typeof message !== 'string') { + return false + } + + return message + .split('\n') + .some((line) => + SUCCESSFUL_EDIT_MESSAGES.some((successMessage) => + line.trim().startsWith(successMessage), + ), + ) +} + +function isErrorOutput(output: string): boolean { + const trimmedOutput = output.trim() + return trimmedOutput.startsWith('Error:') || trimmedOutput.startsWith('Failed ') +} + /** * Construct a simple diff view from str_replace replacements. */ +type ReplacementInput = { + oldString?: string + newString?: string + old?: string + new?: string +} + function constructDiffFromReplacements( - replacements: { old: string; new: string }[], + replacements: ReplacementInput[], ): string { const lines: string[] = [] for (const replacement of replacements) { + const oldString = replacement.oldString ?? replacement.old ?? '' + const newString = replacement.newString ?? replacement.new ?? '' + // Add old lines as removals - const oldLines = replacement.old.split('\n') + const oldLines = oldString.split('\n') for (const line of oldLines) { lines.push(`- ${line}`) } // Add new lines as additions - const newLines = replacement.new.split('\n') + const newLines = newString.split('\n') for (const line of newLines) { lines.push(`+ ${line}`) } @@ -315,7 +424,8 @@ export function isCreateFile(toolBlock: ToolContentBlock): boolean { const message = extractValueForKey(outputStr, 'message') return ( typeof message === 'string' && - (message.startsWith('Created new file') || + (message.startsWith('Created file successfully') || + message.startsWith('Created new file') || message.startsWith('Proposed new file')) ) } @@ -400,7 +510,9 @@ export function getFileChangeType(toolBlock: ToolContentBlock): FileChangeType { * Get aggregated file stats from all edit blocks. * Groups by file path and sums up the stats. */ -export function getFileStatsFromBlocks(blocks: ContentBlock[] | undefined): FileStats[] { +export function getFileStatsFromBlocks( + blocks: ContentBlock[] | undefined, +): FileStats[] { if (!blocks || blocks.length === 0) return [] const fileMap = new Map() @@ -408,8 +520,12 @@ export function getFileStatsFromBlocks(blocks: ContentBlock[] | undefined): File for (const block of blocks) { if ( block.type === 'tool' && - ALL_EDIT_TOOL_NAMES.includes(block.toolName as (typeof ALL_EDIT_TOOL_NAMES)[number]) + ALL_EDIT_TOOL_NAMES.includes( + block.toolName as (typeof ALL_EDIT_TOOL_NAMES)[number], + ) ) { + if (isFailedEditToolBlock(block)) continue + const filePath = extractFilePath(block) if (!filePath) continue @@ -456,8 +572,12 @@ export function buildActivityTimeline( } } else if ( block.type === 'tool' && - ALL_EDIT_TOOL_NAMES.includes(block.toolName as (typeof ALL_EDIT_TOOL_NAMES)[number]) + ALL_EDIT_TOOL_NAMES.includes( + block.toolName as (typeof ALL_EDIT_TOOL_NAMES)[number], + ) ) { + if (isFailedEditToolBlock(block)) continue + const filePath = extractFilePath(block) const diff = extractDiff(block) const isCreate = isCreateFile(block) @@ -519,8 +639,7 @@ export function getMultiPromptProgress( const selectorAgent = blocks.find( (block): block is AgentContentBlock => - block.type === 'agent' && - block.agentType.includes('best-of-n-selector'), + block.type === 'agent' && block.agentType.includes('best-of-n-selector'), ) const isSelecting = selectorAgent?.status === 'running' @@ -562,7 +681,9 @@ function hasSetOutputData(input: unknown): input is SetOutputInput { * Extract the selection reason from multi-prompt agent's set_output block. * set_output wraps data in a 'data' property, so we need to access input.data.reason */ -function extractSelectionReason(blocks: ContentBlock[] | undefined): string | null { +function extractSelectionReason( + blocks: ContentBlock[] | undefined, +): string | null { if (!blocks || blocks.length === 0) return null const setOutputBlock = blocks.find( @@ -604,7 +725,9 @@ export function getMultiPromptPreview( const formattedReason = reason.charAt(0).toUpperCase() + reason.slice(1) const lines = formattedReason.split('\n') const truncatedReason = - lines.length > 2 ? lines.slice(0, 2).join('\n').trimEnd() + '...' : formattedReason + lines.length > 2 + ? lines.slice(0, 2).join('\n').trimEnd() + '...' + : formattedReason return `${total} proposals evaluated\n${truncatedReason}` } return `${total} proposals evaluated` diff --git a/common/src/templates/initial-agents-dir/types/tools.ts b/common/src/templates/initial-agents-dir/types/tools.ts index 9cfe1cdf2e..cb3882fc04 100644 --- a/common/src/templates/initial-agents-dir/types/tools.ts +++ b/common/src/templates/initial-agents-dir/types/tools.ts @@ -226,10 +226,10 @@ export interface ProposeStrReplaceParams { /** Array of replacements to make. */ replacements: { /** The string to replace. This must be an *exact match* of the string you want to replace, including whitespace and punctuation. */ - old: string - /** The string to replace the corresponding old string with. Can be empty to delete. */ - new: string - /** Whether to allow multiple replacements of old string. */ + oldString: string + /** The string to replace the corresponding oldString with. Can be empty to delete. */ + newString: string + /** Whether to allow multiple replacements of oldString. */ allowMultiple?: boolean }[] } @@ -358,10 +358,10 @@ export interface StrReplaceParams { /** Array of replacements to make. */ replacements: { /** The string to replace. This must be an *exact match* of the string you want to replace, including whitespace and punctuation. */ - old: string - /** The string to replace the corresponding old string with. Can be empty to delete. */ - new: string - /** Whether to allow multiple replacements of old string. */ + oldString: string + /** The string to replace the corresponding oldString with. Can be empty to delete. */ + newString: string + /** Whether to allow multiple replacements of oldString. */ allowMultiple?: boolean }[] } diff --git a/common/src/tools/params/__tests__/coerce-to-array.test.ts b/common/src/tools/params/__tests__/coerce-to-array.test.ts index ece3e12c44..ccd80ce6bf 100644 --- a/common/src/tools/params/__tests__/coerce-to-array.test.ts +++ b/common/src/tools/params/__tests__/coerce-to-array.test.ts @@ -135,8 +135,8 @@ describe('normalizeReplacementAliases', () => { ).toEqual({ old_str: 'before', new_str: 'after', - old: 'before', - new: 'after', + oldString: 'before', + newString: 'after', allowMultiple: true, }) }) @@ -150,22 +150,22 @@ describe('normalizeReplacementAliases', () => { ).toEqual({ old_string: 'before', new_string: 'after', - old: 'before', - new: 'after', + oldString: 'before', + newString: 'after', }) }) it('does not overwrite documented replacement keys', () => { expect( normalizeReplacementAliases({ - old: 'before', - new: 'after', + oldString: 'before', + newString: 'after', old_str: 'ignored', new_str: 'ignored', }), ).toEqual({ - old: 'before', - new: 'after', + oldString: 'before', + newString: 'after', old_str: 'ignored', new_str: 'ignored', }) diff --git a/common/src/tools/params/tool/propose-str-replace.ts b/common/src/tools/params/tool/propose-str-replace.ts index d4d7747473..ab86885d7a 100644 --- a/common/src/tools/params/tool/propose-str-replace.ts +++ b/common/src/tools/params/tool/propose-str-replace.ts @@ -38,27 +38,27 @@ const inputSchema = z .preprocess( normalizeReplacementAliases, z.object({ - old: z + oldString: z .string() - .min(1, 'Old cannot be empty') + .min(1, 'oldString cannot be empty') .describe( `The string to replace. This must be an *exact match* of the string you want to replace, including whitespace and punctuation.`, ), - new: z + newString: z .string() .describe( - `The string to replace the corresponding old string with. Can be empty to delete.`, + `The string to replace the corresponding oldString with. Can be empty to delete.`, ), allowMultiple: z .boolean() .optional() .default(false) .describe( - 'Whether to allow multiple replacements of old string.', + 'Whether to allow multiple replacements of oldString.', ), }), ) - .describe('Pair of old and new strings.'), + .describe('Pair of oldString and newString values.'), ) .min(1, 'Replacements cannot be empty'), ) @@ -79,10 +79,13 @@ ${$getNativeToolCallExampleString({ input: { path: 'path/to/file', replacements: [ - { old: 'This is the old string', new: 'This is the new string' }, { - old: '\nfoo:', - new: '\nbar:', + oldString: 'This is the old string', + newString: 'This is the new string', + }, + { + oldString: '\nfoo:', + newString: '\nbar:', allowMultiple: true, }, ], diff --git a/common/src/tools/params/tool/str-replace.ts b/common/src/tools/params/tool/str-replace.ts index 60350a6270..1c697913c9 100644 --- a/common/src/tools/params/tool/str-replace.ts +++ b/common/src/tools/params/tool/str-replace.ts @@ -13,7 +13,6 @@ export const updateFileResultSchema = z.union([ z.object({ file: z.string(), message: z.string(), - unifiedDiff: z.string(), }), z.object({ file: z.string(), @@ -39,27 +38,27 @@ const inputSchema = z .preprocess( normalizeReplacementAliases, z.object({ - old: z + oldString: z .string() - .min(1, 'Old cannot be empty') + .min(1, 'oldString cannot be empty') .describe( `The string to replace. This must be an *exact match* of the string you want to replace, including whitespace and punctuation.`, ), - new: z + newString: z .string() .describe( - `The string to replace the corresponding old string with. Can be empty to delete.`, + `The string to replace the corresponding oldString with. Can be empty to delete.`, ), allowMultiple: z .boolean() .optional() .default(false) .describe( - 'Whether to allow multiple replacements of old string.', + 'Whether to allow multiple replacements of oldString.', ), }), ) - .describe('Pair of old and new strings.'), + .describe('Pair of oldString and newString values.'), ) .min(1, 'Replacements cannot be empty'), ) @@ -79,14 +78,18 @@ ${$getNativeToolCallExampleString({ input: { path: 'path/to/file', replacements: [ - { old: 'This is the old string', new: 'This is the new string' }, { - old: '\n\t\t// @codebuff delete this log line please\n\t\tconsole.log("Hello, world!");\n', - new: '\n', + oldString: 'This is the old string', + newString: 'This is the new string', }, { - old: '\nfoo:', - new: '\nbar:', + oldString: + '\n\t\t// @codebuff delete this log line please\n\t\tconsole.log("Hello, world!");\n', + newString: '\n', + }, + { + oldString: '\nfoo:', + newString: '\nbar:', allowMultiple: true, }, ], diff --git a/common/src/tools/params/utils.ts b/common/src/tools/params/utils.ts index 870d7c76ca..9b275aa8c2 100644 --- a/common/src/tools/params/utils.ts +++ b/common/src/tools/params/utils.ts @@ -43,8 +43,8 @@ export function normalizeReplacementAliases(val: unknown): unknown { const replacement = { ...(val as Record) } for (const [target, aliases] of [ - ['old', ['old_str', 'old_string']], - ['new', ['new_str', 'new_string']], + ['oldString', ['old', 'old_str', 'old_string']], + ['newString', ['new', 'new_str', 'new_string']], ] as const) { if (replacement[target] !== undefined) { continue diff --git a/packages/agent-runtime/src/__tests__/process-str-replace.test.ts b/packages/agent-runtime/src/__tests__/process-str-replace.test.ts index aa8392e256..b7e7fd4956 100644 --- a/packages/agent-runtime/src/__tests__/process-str-replace.test.ts +++ b/packages/agent-runtime/src/__tests__/process-str-replace.test.ts @@ -20,7 +20,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: false }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: false }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -41,7 +43,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: false }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: false }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -61,7 +65,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: false }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: false }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -80,7 +86,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: false }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: false }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -95,7 +103,9 @@ describe('processStrReplace', () => { it('should return error if file content is null and oldStr is not empty', async () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: 'old', new: 'new', allowMultiple: false }], + replacements: [ + { oldString: 'old', newString: 'new', allowMultiple: false }, + ], initialContentPromise: Promise.resolve(null), logger, }) @@ -110,7 +120,7 @@ describe('processStrReplace', () => { it('should return error if oldStr is empty and file exists', async () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: '', new: 'new', allowMultiple: false }], + replacements: [{ oldString: '', newString: 'new', allowMultiple: false }], initialContentPromise: Promise.resolve('content'), logger, }) @@ -129,7 +139,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: false }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: false }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -150,7 +162,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: true }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: true }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -169,7 +183,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: false }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: false }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -191,7 +207,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: false }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: false }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -208,9 +226,21 @@ describe('processStrReplace', () => { it('should continue processing other replacements even if one fails', async () => { const initialContent = 'const x = 1;\nconst y = 2;\nconst z = 3;\n' const replacements = [ - { old: 'const x = 1;', new: 'const x = 10;', allowMultiple: false }, // This exists - { old: 'const w = 4;', new: 'const w = 40;', allowMultiple: false }, // This doesn't exist - { old: 'const z = 3;', new: 'const z = 30;', allowMultiple: false }, // This also exists + { + oldString: 'const x = 1;', + newString: 'const x = 10;', + allowMultiple: false, + }, // This exists + { + oldString: 'const w = 4;', + newString: 'const w = 40;', + allowMultiple: false, + }, // This doesn't exist + { + oldString: 'const z = 3;', + newString: 'const z = 30;', + allowMultiple: false, + }, // This also exists ] const result = await processStrReplace({ @@ -242,7 +272,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: false }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: false }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -262,7 +294,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: true }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: true }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -281,7 +315,9 @@ describe('processStrReplace', () => { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: true }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: true }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -296,9 +332,9 @@ describe('processStrReplace', () => { it('should handle mixed allowMultiple settings in multiple replacements', async () => { const initialContent = 'foo bar foo\nbaz baz baz\nqux qux' const replacements = [ - { old: 'foo', new: 'FOO', allowMultiple: true }, // Replace all 'foo' - { old: 'baz', new: 'BAZ', allowMultiple: false }, // Should error on multiple 'baz' - { old: 'qux qux', new: 'QUX', allowMultiple: false }, // Single occurrence, should work + { oldString: 'foo', newString: 'FOO', allowMultiple: true }, // Replace all 'foo' + { oldString: 'baz', newString: 'BAZ', allowMultiple: false }, // Should error on multiple 'baz' + { oldString: 'qux qux', newString: 'QUX', allowMultiple: false }, // Single occurrence, should work ] const result = await processStrReplace({ @@ -335,7 +371,9 @@ function test3() { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: true }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: true }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -359,7 +397,9 @@ function test3() { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: true }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: true }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -383,7 +423,9 @@ function test3() { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: true }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: true }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -403,7 +445,9 @@ function test3() { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: true }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: true }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) @@ -422,13 +466,13 @@ function test3() { const initialContent = 'line 1\nline 2\nline 3\n' const replacements = [ { - old: 'line 2\n', - new: 'this is a new line\n', + oldString: 'line 2\n', + newString: 'this is a new line\n', allowMultiple: false, }, { - old: 'line 3\n', - new: 'new line 3\n', + oldString: 'line 3\n', + newString: 'new line 3\n', allowMultiple: false, }, ] @@ -454,7 +498,9 @@ function test3() { const result = await processStrReplace({ path: 'test.ts', - replacements: [{ old: oldStr, new: newStr, allowMultiple: false }], + replacements: [ + { oldString: oldStr, newString: newStr, allowMultiple: false }, + ], initialContentPromise: Promise.resolve(initialContent), logger, }) diff --git a/packages/agent-runtime/src/__tests__/propose-tools.test.ts b/packages/agent-runtime/src/__tests__/propose-tools.test.ts index 84ceafb071..55ae16f4d9 100644 --- a/packages/agent-runtime/src/__tests__/propose-tools.test.ts +++ b/packages/agent-runtime/src/__tests__/propose-tools.test.ts @@ -1,10 +1,7 @@ import { TEST_USER_ID } from '@codebuff/common/old-constants' import { TEST_AGENT_RUNTIME_IMPL } from '@codebuff/common/testing/impl/agent-runtime' import { getInitialSessionState } from '@codebuff/common/types/session-state' -import { - assistantMessage, - userMessage, -} from '@codebuff/common/util/messages' +import { assistantMessage, userMessage } from '@codebuff/common/util/messages' import { afterEach, beforeEach, @@ -51,7 +48,9 @@ describe('propose_str_replace and propose_write_file tools', () => { let mockTemplate: AgentTemplate let mockAgentState: AgentState let mockParams: ParamsOf - let executeToolCallSpy: ReturnType> + let executeToolCallSpy: ReturnType< + typeof spyOn + > let agentRuntimeImpl: AgentRuntimeDeps & AgentRuntimeScopedDeps // Mock file system - maps file paths to their contents @@ -59,7 +58,8 @@ describe('propose_str_replace and propose_write_file tools', () => { beforeEach(() => { // Reset mock file system - mockFiles['src/utils.ts'] = `export function add(a: number, b: number): number { + mockFiles['src/utils.ts'] = + `export function add(a: number, b: number): number { return a + b; } @@ -87,18 +87,27 @@ console.log(add(1, 2)); if (toolName === 'propose_str_replace') { const { path, replacements } = input as { path: string - replacements: Array<{ old: string; new: string; allowMultiple: boolean }> + replacements: Array<{ + oldString: string + newString: string + allowMultiple: boolean + }> } - + // Get current content (from proposed state or mock files) let content = mockFiles[path] ?? null - + if (content === null) { const errorResult: ToolMessage = { role: 'tool', toolName: 'propose_str_replace', toolCallId: `${toolName}-call-id`, - content: [{ type: 'json', value: { file: path, errorMessage: `File not found: ${path}` } }], + content: [ + { + type: 'json', + value: { file: path, errorMessage: `File not found: ${path}` }, + }, + ], } toolResults.push(errorResult) agentState.messageHistory.push(errorResult) @@ -108,14 +117,22 @@ console.log(add(1, 2)); // Apply replacements const errors: string[] = [] for (const replacement of replacements) { - if (!content.includes(replacement.old)) { - errors.push(`String not found: "${replacement.old.slice(0, 50)}..."`) + if (!content.includes(replacement.oldString)) { + errors.push( + `String not found: "${replacement.oldString.slice(0, 50)}..."`, + ) continue } if (replacement.allowMultiple) { - content = content.replaceAll(replacement.old, replacement.new) + content = content.replaceAll( + replacement.oldString, + replacement.newString, + ) } else { - content = content.replace(replacement.old, replacement.new) + content = content.replace( + replacement.oldString, + replacement.newString, + ) } } @@ -124,7 +141,12 @@ console.log(add(1, 2)); role: 'tool', toolName: 'propose_str_replace', toolCallId: `${toolName}-call-id`, - content: [{ type: 'json', value: { file: path, errorMessage: errors.join('; ') } }], + content: [ + { + type: 'json', + value: { file: path, errorMessage: errors.join('; ') }, + }, + ], } toolResults.push(errorResult) agentState.messageHistory.push(errorResult) @@ -134,7 +156,7 @@ console.log(add(1, 2)); // Generate unified diff const originalContent = mockFiles[path]! const diff = generateSimpleDiff(path, originalContent, content) - + // Store proposed content for future calls mockFiles[path] = content @@ -142,14 +164,16 @@ console.log(add(1, 2)); role: 'tool', toolName: 'propose_str_replace', toolCallId: `${toolName}-call-id`, - content: [{ - type: 'json', - value: { - file: path, - message: 'Proposed string replacements', - unifiedDiff: diff, + content: [ + { + type: 'json', + value: { + file: path, + message: 'Proposed string replacements', + unifiedDiff: diff, + }, }, - }], + ], } toolResults.push(successResult) agentState.messageHistory.push(successResult) @@ -159,13 +183,13 @@ console.log(add(1, 2)); instructions: string content: string } - + const originalContent = mockFiles[path] ?? '' const isNewFile = !(path in mockFiles) - + // Generate unified diff const diff = generateSimpleDiff(path, originalContent, newContent) - + // Store proposed content mockFiles[path] = newContent @@ -173,14 +197,18 @@ console.log(add(1, 2)); role: 'tool', toolName: 'propose_write_file', toolCallId: `${toolName}-call-id`, - content: [{ - type: 'json', - value: { - file: path, - message: isNewFile ? `Proposed new file ${path}` : `Proposed changes to ${path}`, - unifiedDiff: diff, + content: [ + { + type: 'json', + value: { + file: path, + message: isNewFile + ? `Proposed new file ${path}` + : `Proposed changes to ${path}`, + unifiedDiff: diff, + }, }, - }], + ], } toolResults.push(successResult) agentState.messageHistory.push(successResult) @@ -201,7 +229,8 @@ console.log(add(1, 2)); // Mock crypto.randomUUID spyOn(crypto, 'randomUUID').mockImplementation( - () => 'mock-uuid-0000-0000-0000-000000000000' as `${string}-${string}-${string}-${string}-${string}`, + () => + 'mock-uuid-0000-0000-0000-000000000000' as `${string}-${string}-${string}-${string}-${string}`, ) // Create mock template for implementor agent @@ -215,10 +244,16 @@ console.log(add(1, 2)); includeMessageHistory: true, inheritParentSystemPrompt: false, mcpServers: {}, - toolNames: ['propose_str_replace', 'propose_write_file', 'set_output', 'end_turn'], + toolNames: [ + 'propose_str_replace', + 'propose_write_file', + 'set_output', + 'end_turn', + ], spawnableAgents: [], systemPrompt: 'You are a code implementor that proposes changes.', - instructionsPrompt: 'Implement the requested changes using propose_str_replace or propose_write_file.', + instructionsPrompt: + 'Implement the requested changes using propose_str_replace or propose_write_file.', stepPrompt: '', handleSteps: undefined, } as AgentTemplate @@ -228,7 +263,8 @@ console.log(add(1, 2)); mockAgentState = { ...sessionState.mainAgentState, agentId: 'test-implementor-id', - runId: 'test-run-id' as `${string}-${string}-${string}-${string}-${string}`, + runId: + 'test-run-id' as `${string}-${string}-${string}-${string}-${string}`, messageHistory: [ userMessage('Add a multiply function to src/utils.ts'), assistantMessage('I will implement the changes.'), @@ -281,23 +317,29 @@ console.log(add(1, 2)); toolName: 'propose_str_replace', input: { path: 'src/utils.ts', - replacements: [{ - old: 'export function subtract(a: number, b: number): number {\n return a - b;\n}', - new: `export function subtract(a: number, b: number): number { + replacements: [ + { + oldString: + 'export function subtract(a: number, b: number): number {\n return a - b;\n}', + newString: `export function subtract(a: number, b: number): number { return a - b; } export function multiply(a: number, b: number): number { return a * b; }`, - allowMultiple: false, - }], + allowMultiple: false, + }, + ], }, } toolResultsCapture.push(step.toolResult) - + const firstResult = step.toolResult?.[0] - const unifiedDiff = firstResult?.type === 'json' ? (firstResult.value as { unifiedDiff?: string })?.unifiedDiff : undefined + const unifiedDiff = + firstResult?.type === 'json' + ? (firstResult.value as { unifiedDiff?: string })?.unifiedDiff + : undefined yield { toolName: 'set_output', input: { @@ -325,9 +367,14 @@ export function multiply(a: number, b: number): number { const toolResult = toolResultsCapture[0] expect(toolResult).toBeDefined() expect(toolResult[0].type).toBe('json') - const jsonResult = toolResult[0] as { type: 'json'; value: { file: string; unifiedDiff: string } } + const jsonResult = toolResult[0] as { + type: 'json' + value: { file: string; unifiedDiff: string } + } expect(jsonResult.value.file).toBe('src/utils.ts') - expect(jsonResult.value.unifiedDiff).toContain('+export function multiply') + expect(jsonResult.value.unifiedDiff).toContain( + '+export function multiply', + ) expect(jsonResult.value.unifiedDiff).toContain('return a * b') }) @@ -339,11 +386,13 @@ export function multiply(a: number, b: number): number { toolName: 'propose_str_replace', input: { path: 'src/utils.ts', - replacements: [{ - old: 'nonexistent string that does not exist in the file', - new: 'replacement', - allowMultiple: false, - }], + replacements: [ + { + oldString: 'nonexistent string that does not exist in the file', + newString: 'replacement', + allowMultiple: false, + }, + ], }, } toolResultsCapture.push(step.toolResult) @@ -356,7 +405,10 @@ export function multiply(a: number, b: number): number { expect(toolResultsCapture).toHaveLength(1) const toolResult = toolResultsCapture[0] - const jsonResult = toolResult[0] as { type: 'json'; value: { errorMessage: string } } + const jsonResult = toolResult[0] as { + type: 'json' + value: { errorMessage: string } + } expect(jsonResult.value.errorMessage).toContain('String not found') }) @@ -369,11 +421,13 @@ export function multiply(a: number, b: number): number { toolName: 'propose_str_replace', input: { path: 'src/utils.ts', - replacements: [{ - old: 'return a + b;', - new: 'return a + b; // addition', - allowMultiple: false, - }], + replacements: [ + { + oldString: 'return a + b;', + newString: 'return a + b; // addition', + allowMultiple: false, + }, + ], }, } toolResultsCapture.push({ step: 1, result: step1.toolResult }) @@ -383,11 +437,13 @@ export function multiply(a: number, b: number): number { toolName: 'propose_str_replace', input: { path: 'src/utils.ts', - replacements: [{ - old: 'return a - b;', - new: 'return a - b; // subtraction', - allowMultiple: false, - }], + replacements: [ + { + oldString: 'return a - b;', + newString: 'return a - b; // subtraction', + allowMultiple: false, + }, + ], }, } toolResultsCapture.push({ step: 2, result: step2.toolResult }) @@ -400,13 +456,19 @@ export function multiply(a: number, b: number): number { await runProgrammaticStep(mockParams) expect(toolResultsCapture).toHaveLength(2) - + // Both replacements should succeed - const result0 = toolResultsCapture[0].result[0] as { type: 'json'; value: { unifiedDiff: string } } - const result1 = toolResultsCapture[1].result[0] as { type: 'json'; value: { unifiedDiff: string } } + const result0 = toolResultsCapture[0].result[0] as { + type: 'json' + value: { unifiedDiff: string } + } + const result1 = toolResultsCapture[1].result[0] as { + type: 'json' + value: { unifiedDiff: string } + } expect(result0.value.unifiedDiff).toContain('// addition') expect(result1.value.unifiedDiff).toContain('// subtraction') - + // Final file should have both changes expect(mockFiles['src/utils.ts']).toContain('// addition') expect(mockFiles['src/utils.ts']).toContain('// subtraction') @@ -439,10 +501,15 @@ export function multiply(a: number, b: number): number { expect(toolResultsCapture).toHaveLength(1) const toolResult = toolResultsCapture[0] - const jsonResult = toolResult[0] as { type: 'json'; value: { file: string; message: string; unifiedDiff: string } } + const jsonResult = toolResult[0] as { + type: 'json' + value: { file: string; message: string; unifiedDiff: string } + } expect(jsonResult.value.file).toBe('src/multiply.ts') expect(jsonResult.value.message).toContain('new file') - expect(jsonResult.value.unifiedDiff).toContain('+export function multiply') + expect(jsonResult.value.unifiedDiff).toContain( + '+export function multiply', + ) }) it('should propose file edit and return unified diff', async () => { @@ -478,10 +545,15 @@ export function multiply(a: number, b: number): number { expect(toolResultsCapture).toHaveLength(1) const toolResult = toolResultsCapture[0] - const jsonResult = toolResult[0] as { type: 'json'; value: { file: string; message: string; unifiedDiff: string } } + const jsonResult = toolResult[0] as { + type: 'json' + value: { file: string; message: string; unifiedDiff: string } + } expect(jsonResult.value.file).toBe('src/utils.ts') expect(jsonResult.value.message).toContain('changes') - expect(jsonResult.value.unifiedDiff).toContain('+export function multiply') + expect(jsonResult.value.unifiedDiff).toContain( + '+export function multiply', + ) }) }) @@ -501,15 +573,19 @@ export function multiply(a: number, b: number): number { toolName: 'propose_str_replace', input: { path: 'src/utils.ts', - replacements: [{ - old: 'return a + b;', - new: 'return a + b; // first change', - allowMultiple: false, - }], + replacements: [ + { + oldString: 'return a + b;', + newString: 'return a + b; // first change', + allowMultiple: false, + }, + ], }, } const step1First = step1.toolResult?.[0] - const step1HasDiff = step1First?.type === 'json' && !!(step1First.value as { unifiedDiff?: string })?.unifiedDiff + const step1HasDiff = + step1First?.type === 'json' && + !!(step1First.value as { unifiedDiff?: string })?.unifiedDiff receivedToolResults.push({ step: 1, toolResult: step1.toolResult, @@ -521,15 +597,19 @@ export function multiply(a: number, b: number): number { toolName: 'propose_str_replace', input: { path: 'src/utils.ts', - replacements: [{ - old: 'return a - b;', - new: 'return a - b; // second change', - allowMultiple: false, - }], + replacements: [ + { + oldString: 'return a - b;', + newString: 'return a - b; // second change', + allowMultiple: false, + }, + ], }, } const step2First = step2.toolResult?.[0] - const step2HasDiff = step2First?.type === 'json' && !!(step2First.value as { unifiedDiff?: string })?.unifiedDiff + const step2HasDiff = + step2First?.type === 'json' && + !!(step2First.value as { unifiedDiff?: string })?.unifiedDiff receivedToolResults.push({ step: 2, toolResult: step2.toolResult, @@ -546,7 +626,9 @@ export function multiply(a: number, b: number): number { }, } const step3First = step3.toolResult?.[0] - const step3HasDiff = step3First?.type === 'json' && !!(step3First.value as { unifiedDiff?: string })?.unifiedDiff + const step3HasDiff = + step3First?.type === 'json' && + !!(step3First.value as { unifiedDiff?: string })?.unifiedDiff receivedToolResults.push({ step: 3, toolResult: step3.toolResult, @@ -561,31 +643,40 @@ export function multiply(a: number, b: number): number { const result = await runProgrammaticStep(mockParams) expect(result.endTurn).toBe(true) - + // Verify we received tool results for all 3 steps expect(receivedToolResults).toHaveLength(3) - + // Step 1: Should have received tool result with unified diff expect(receivedToolResults[0].step).toBe(1) expect(receivedToolResults[0].toolResult).toBeDefined() expect(receivedToolResults[0].hasUnifiedDiff).toBe(true) - const step1Result = receivedToolResults[0].toolResult[0] as { type: 'json'; value: { file: string; unifiedDiff: string } } + const step1Result = receivedToolResults[0].toolResult[0] as { + type: 'json' + value: { file: string; unifiedDiff: string } + } expect(step1Result.value.file).toBe('src/utils.ts') expect(step1Result.value.unifiedDiff).toContain('first change') - + // Step 2: Should have received tool result with unified diff expect(receivedToolResults[1].step).toBe(2) expect(receivedToolResults[1].toolResult).toBeDefined() expect(receivedToolResults[1].hasUnifiedDiff).toBe(true) - const step2Result = receivedToolResults[1].toolResult[0] as { type: 'json'; value: { file: string; unifiedDiff: string } } + const step2Result = receivedToolResults[1].toolResult[0] as { + type: 'json' + value: { file: string; unifiedDiff: string } + } expect(step2Result.value.file).toBe('src/utils.ts') expect(step2Result.value.unifiedDiff).toContain('second change') - + // Step 3: Should have received tool result with unified diff for new file expect(receivedToolResults[2].step).toBe(3) expect(receivedToolResults[2].toolResult).toBeDefined() expect(receivedToolResults[2].hasUnifiedDiff).toBe(true) - const step3Result = receivedToolResults[2].toolResult[0] as { type: 'json'; value: { file: string; message: string } } + const step3Result = receivedToolResults[2].toolResult[0] as { + type: 'json' + value: { file: string; message: string } + } expect(step3Result.value.file).toBe('src/new-file.ts') expect(step3Result.value.message).toContain('new file') }) @@ -607,20 +698,23 @@ export function multiply(a: number, b: number): number { toolName: 'propose_str_replace', input: { path: 'src/utils.ts', - replacements: [{ - old: 'export function subtract(a: number, b: number): number {\n return a - b;\n}', - new: `export function subtract(a: number, b: number): number { + replacements: [ + { + oldString: + 'export function subtract(a: number, b: number): number {\n return a - b;\n}', + newString: `export function subtract(a: number, b: number): number { return a - b; } export function multiply(a: number, b: number): number { return a * b; }`, - allowMultiple: false, - }], + allowMultiple: false, + }, + ], }, } - + // Capture the tool call and result capturedToolCalls.push({ toolName: 'propose_str_replace', @@ -654,7 +748,7 @@ export function multiply(a: number, b: number): number { expect(result.endTurn).toBe(true) expect(result.agentState.output).toBeDefined() - + const output = result.agentState.output as { toolCalls: any[] toolResults: any[] @@ -668,7 +762,9 @@ export function multiply(a: number, b: number): number { // Verify tool results were captured expect(output.toolResults).toHaveLength(1) expect(output.toolResults[0].file).toBe('src/utils.ts') - expect(output.toolResults[0].unifiedDiff).toContain('+export function multiply') + expect(output.toolResults[0].unifiedDiff).toContain( + '+export function multiply', + ) // Verify unified diffs string was generated expect(output.unifiedDiffs).toContain('--- src/utils.ts ---') @@ -681,25 +777,31 @@ export function multiply(a: number, b: number): number { * Simple diff generator for testing purposes. * In production, the actual handlers use the 'diff' library. */ -function generateSimpleDiff(path: string, oldContent: string, newContent: string): string { +function generateSimpleDiff( + path: string, + oldContent: string, + newContent: string, +): string { const oldLines = oldContent.split('\n') const newLines = newContent.split('\n') - + const diffLines: string[] = [] const maxLen = Math.max(oldLines.length, newLines.length) - + let inChange = false let _changeStart = 0 - + for (let i = 0; i < maxLen; i++) { const oldLine = oldLines[i] const newLine = newLines[i] - + if (oldLine !== newLine) { if (!inChange) { inChange = true _changeStart = i - diffLines.push(`@@ -${i + 1},${oldLines.length - i} +${i + 1},${newLines.length - i} @@`) + diffLines.push( + `@@ -${i + 1},${oldLines.length - i} +${i + 1},${newLines.length - i} @@`, + ) } if (oldLine !== undefined) { diffLines.push(`-${oldLine}`) @@ -711,6 +813,6 @@ function generateSimpleDiff(path: string, oldContent: string, newContent: string diffLines.push(` ${oldLine}`) } } - + return diffLines.join('\n') } diff --git a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts index 9b834024ac..ddba9b8dae 100644 --- a/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts +++ b/packages/agent-runtime/src/__tests__/tool-validation-error.test.ts @@ -144,7 +144,32 @@ describe('tool validation error handling', () => { expect('error' in result).toBe(false) if (!('error' in result)) { expect(result.input.replacements).toEqual([ - { old: 'before', new: 'after', allowMultiple: false }, + { oldString: 'before', newString: 'after', allowMultiple: false }, + ]) + } + }) + + it('should accept old/new aliases for str_replace replacements', () => { + const result = parseRawToolCall({ + rawToolCall: { + toolName: 'str_replace', + toolCallId: 'short-alias-tool-call-id', + input: { + path: 'test.ts', + replacements: [ + { + old: 'before', + new: 'after', + }, + ], + }, + }, + }) + + expect('error' in result).toBe(false) + if (!('error' in result)) { + expect(result.input.replacements).toEqual([ + { oldString: 'before', newString: 'after', allowMultiple: false }, ]) } }) @@ -169,7 +194,7 @@ describe('tool validation error handling', () => { expect('error' in result).toBe(false) if (!('error' in result)) { expect(result.input.replacements).toEqual([ - { old: 'before', new: 'after', allowMultiple: false }, + { oldString: 'before', newString: 'after', allowMultiple: false }, ]) } }) @@ -182,9 +207,9 @@ describe('tool validation error handling', () => { input: { path: 'test.ts', replacements: [ - { old: 'before', new: 'after' }, - { old: 'delete me' }, - { old: 'delete me too' }, + { oldString: 'before', newString: 'after' }, + { oldString: 'delete me' }, + { oldString: 'delete me too' }, ], }, }, @@ -193,10 +218,10 @@ describe('tool validation error handling', () => { expect('error' in result).toBe(true) if ('error' in result) { expect(result.error).toContain('Missing required replacement fields:') - expect(result.error).toContain('- replacements[1].new') - expect(result.error).toContain('- replacements[2].new') + expect(result.error).toContain('- replacements[1].newString') + expect(result.error).toContain('- replacements[2].newString') expect(result.error).toContain( - 'If the intent is deletion, set "new": "" explicitly.', + 'If the intent is deletion, set "newString": "" explicitly.', ) expect(result.error).toContain('Raw validation issues:') } diff --git a/packages/agent-runtime/src/process-str-replace.ts b/packages/agent-runtime/src/process-str-replace.ts index 12d25d48de..e836b77fd9 100644 --- a/packages/agent-runtime/src/process-str-replace.ts +++ b/packages/agent-runtime/src/process-str-replace.ts @@ -10,7 +10,11 @@ function normalizeLineEndings(params: { str: string }): string { export async function processStrReplace(params: { path: string - replacements: { old: string; new: string; allowMultiple: boolean }[] + replacements: { + oldString: string + newString: string + allowMultiple: boolean + }[] initialContentPromise: Promise logger: Logger }): Promise< @@ -34,12 +38,16 @@ export async function processStrReplace(params: { } } - // Process each old/new string pair + // Process each oldString/newString pair let currentContent = initialContent let messages: string[] = [] const lineEnding = currentContent.includes('\r\n') ? '\r\n' : '\n' - for (const { old: oldStr, new: newStr, allowMultiple } of replacements) { + for (const { + oldString: oldStr, + newString: newStr, + allowMultiple, + } of replacements) { // Regular case: require oldStr for replacements if (!oldStr) { messages.push( diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index 303765ea7d..3980ad9415 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -161,7 +161,7 @@ function summarizeMissingReplacementFields( issue.message?.includes('received undefined') && root === 'replacements' && typeof index === 'number' && - (field === 'old' || field === 'new') + (field === 'oldString' || field === 'newString') return isMissingReplacementString ? [`replacements[${index}].${field}`] : [] }) @@ -174,13 +174,13 @@ function summarizeMissingReplacementFields( 'Missing required replacement fields:', ...missingFields.map((field) => `- ${field}`), '', - 'If the intent is deletion, set "new": "" explicitly.', + 'If the intent is deletion, set "newString": "" explicitly.', ].join('\n') } function getToolValidationHint(toolName: string): string | undefined { if (toolName === 'str_replace' || toolName === 'propose_str_replace') { - return 'Expected shape: { "path": string, "replacements": [{ "old": string, "new": string, "allowMultiple"?: boolean }] }.' + return 'Expected shape: { "path": string, "replacements": [{ "oldString": string, "newString": string, "allowMultiple"?: boolean }] }.' } if (toolName === 'write_file' || toolName === 'propose_write_file') { return 'Expected shape: { "path": string, "instructions": string, "content": string }. Quote string values and escape newlines/quotes inside content.' diff --git a/packages/agent-runtime/src/util/__tests__/parse-tool-calls-from-text.test.ts b/packages/agent-runtime/src/util/__tests__/parse-tool-calls-from-text.test.ts index a61e82703f..7b182237b0 100644 --- a/packages/agent-runtime/src/util/__tests__/parse-tool-calls-from-text.test.ts +++ b/packages/agent-runtime/src/util/__tests__/parse-tool-calls-from-text.test.ts @@ -39,7 +39,7 @@ Some text between { "cb_tool_name": "str_replace", "path": "file1.ts", - "replacements": [{"old": "foo", "new": "bar"}] + "replacements": [{"oldString": "foo", "newString": "bar"}] } @@ -56,7 +56,7 @@ Some commentary after` toolName: 'str_replace', input: { path: 'file1.ts', - replacements: [{ old: 'foo', new: 'bar' }], + replacements: [{ oldString: 'foo', newString: 'bar' }], }, }) }) @@ -178,7 +178,7 @@ Some commentary after` '{\n' + ' "cb_tool_name": "str_replace",\n' + ' "path": "test.ts",\n' + - ' "replacements": [{"old": "console.log(\\"hello\\")", "new": "console.log(\'world\')"}]\n' + + ' "replacements": [{"oldString": "console.log(\\"hello\\")", "newString": "console.log(\'world\')"}]\n' + '}\n' + '' @@ -186,10 +186,10 @@ Some commentary after` expect(result).toHaveLength(1) const replacements = result[0].input.replacements as Array<{ - old: string - new: string + oldString: string + newString: string }> - expect(replacements[0].old).toBe('console.log("hello")') + expect(replacements[0].oldString).toBe('console.log("hello")') }) it('should handle tool calls with newlines in content', () => { diff --git a/sdk/src/__tests__/change-file.test.ts b/sdk/src/__tests__/change-file.test.ts new file mode 100644 index 0000000000..dff8969c7e --- /dev/null +++ b/sdk/src/__tests__/change-file.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, test } from 'bun:test' + +import { createMockFs } from '@codebuff/common/testing/mocks/filesystem' + +import { changeFile } from '../tools/change-file' + +describe('changeFile', () => { + test('returns a simple success message for string replacements', async () => { + const fs = createMockFs({ + files: { + '/repo/src/file.ts': 'const value = 1\n', + }, + }) + + const result = await changeFile({ + parameters: { + type: 'patch', + path: 'src/file.ts', + content: '@@ -1,1 +1,1 @@\n-const value = 1\n+const value = 2\n', + }, + cwd: '/repo', + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + file: 'src/file.ts', + message: 'String replace applied successfully.', + }, + }, + ]) + expect(await fs.readFile('/repo/src/file.ts', 'utf-8')).toBe( + 'const value = 2\n', + ) + }) + + test('returns a simple success message for new file writes', async () => { + const fs = createMockFs() + + const result = await changeFile({ + parameters: { + type: 'file', + path: 'src/file.ts', + content: 'const value = 1\n', + }, + cwd: '/repo', + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + file: 'src/file.ts', + message: 'Created file successfully.', + }, + }, + ]) + expect(await fs.readFile('/repo/src/file.ts', 'utf-8')).toBe( + 'const value = 1\n', + ) + }) + + test('returns a simple success message for overwritten file writes', async () => { + const fs = createMockFs({ + files: { + '/repo/src/file.ts': 'const value = 1\n', + }, + }) + + const result = await changeFile({ + parameters: { + type: 'file', + path: 'src/file.ts', + content: 'const value = 2\n', + }, + cwd: '/repo', + fs, + }) + + expect(result).toEqual([ + { + type: 'json', + value: { + file: 'src/file.ts', + message: 'Overwrote file successfully.', + }, + }, + ]) + expect(await fs.readFile('/repo/src/file.ts', 'utf-8')).toBe( + 'const value = 2\n', + ) + }) +}) diff --git a/sdk/src/tools/change-file.ts b/sdk/src/tools/change-file.ts index da372e7dbc..ff34cc547a 100644 --- a/sdk/src/tools/change-file.ts +++ b/sdk/src/tools/change-file.ts @@ -4,7 +4,6 @@ import { fileExists } from '@codebuff/common/util/file' import { applyPatch } from 'diff' import z from 'zod/v4' - import type { CodebuffToolOutput } from '@codebuff/common/tools/list' import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem' @@ -43,7 +42,6 @@ export async function changeFile(params: { if (containsPathTraversal(fileChange.path)) { throw new Error('file path contains invalid path traversal') } - const lines = fileChange.content.split('\n') const { created, modified, invalid, patchFailed } = await applyChanges({ projectRoot: cwd, @@ -56,16 +54,20 @@ export async function changeFile(params: { for (const file of created) { results.push({ file, - message: 'Created new file', - unifiedDiff: lines.join('\n'), + message: + fileChange.type === 'patch' + ? 'String replace applied successfully.' + : 'Created file successfully.', }) } for (const file of modified) { results.push({ file, - message: 'Updated file', - unifiedDiff: lines.join('\n'), + message: + fileChange.type === 'patch' + ? 'String replace applied successfully.' + : 'Overwrote file successfully.', }) } @@ -73,7 +75,7 @@ export async function changeFile(params: { results.push({ file, errorMessage: `Failed to apply patch.`, - patch: lines.join('\n'), + patch: fileChange.content, }) }