From b4fdeb8e21c19a7d0901193ca43e2ab68b5096b6 Mon Sep 17 00:00:00 2001 From: James Grugett Date: Tue, 5 May 2026 12:06:05 -0700 Subject: [PATCH 1/7] Update str_replace argument names --- agents-graveyard/editor/reviewer-editor.ts | 8 +- agents/editor/best-of-n/editor-implementor.ts | 17 +- agents/editor/editor.ts | 8 +- cli/src/components/tools/str-replace.tsx | 2 +- .../__tests__/implementor-helpers.test.ts | 396 +++++++++++------- cli/src/utils/implementor-helpers.ts | 41 +- .../initial-agents-dir/types/tools.ts | 16 +- .../params/__tests__/coerce-to-array.test.ts | 16 +- .../tools/params/tool/propose-str-replace.ts | 21 +- common/src/tools/params/tool/str-replace.ts | 27 +- common/src/tools/params/utils.ts | 4 +- .../src/__tests__/process-str-replace.test.ts | 102 +++-- .../src/__tests__/propose-tools.test.ts | 312 +++++++++----- .../__tests__/tool-validation-error.test.ts | 41 +- .../agent-runtime/src/process-str-replace.ts | 14 +- .../agent-runtime/src/tools/tool-executor.ts | 6 +- .../parse-tool-calls-from-text.test.ts | 12 +- sdk/src/__tests__/change-file.test.ts | 65 +++ sdk/src/tools/change-file.ts | 14 +- 19 files changed, 752 insertions(+), 370 deletions(-) create mode 100644 sdk/src/__tests__/change-file.test.ts 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/cli/src/components/tools/str-replace.tsx b/cli/src/components/tools/str-replace.tsx index 881152472e..4391996713 100644 --- a/cli/src/components/tools/str-replace.tsx +++ b/cli/src/components/tools/str-replace.tsx @@ -73,7 +73,7 @@ const EditBody = ({ name, filePath, diffText, isCreate }: EditBodyProps) => { return ( - {!isCreate && ( + {!isCreate && diffText.length > 0 && ( diff --git a/cli/src/utils/__tests__/implementor-helpers.test.ts b/cli/src/utils/__tests__/implementor-helpers.test.ts index 83bcf2490f..0f49973495 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) @@ -131,9 +134,7 @@ describe('extractDiff', () => { 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 +179,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', () => { @@ -264,7 +273,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) @@ -358,16 +369,30 @@ describe('buildActivityTimeline', () => { 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 +401,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 +451,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 +467,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 +480,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 +596,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 +608,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 +730,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 +858,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 +966,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 +1032,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 +1043,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 +1064,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 +1093,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 +1105,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 +1119,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..996bb6957c 100644 --- a/cli/src/utils/implementor-helpers.ts +++ b/cli/src/utils/implementor-helpers.ts @@ -252,7 +252,7 @@ export function extractDiff(toolBlock: ToolContentBlock): string | null { // 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) } @@ -274,19 +274,29 @@ export function extractDiff(toolBlock: ToolContentBlock): string | null { /** * 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}`) } @@ -400,7 +410,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,7 +420,9 @@ 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], + ) ) { const filePath = extractFilePath(block) if (!filePath) continue @@ -456,7 +470,9 @@ 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], + ) ) { const filePath = extractFilePath(block) const diff = extractDiff(block) @@ -519,8 +535,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 +577,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 +621,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..9c50ed089e --- /dev/null +++ b/sdk/src/__tests__/change-file.test.ts @@ -0,0 +1,65 @@ +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 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: 'Wrote file successfully.', + }, + }, + ]) + expect(await fs.readFile('/repo/src/file.ts', 'utf-8')).toBe( + 'const value = 1\n', + ) + }) +}) diff --git a/sdk/src/tools/change-file.ts b/sdk/src/tools/change-file.ts index da372e7dbc..12c5ddb0fb 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, @@ -52,20 +50,22 @@ export async function changeFile(params: { }) const results: CodebuffToolOutput<'str_replace'>[0]['value'][] = [] + const successMessage = + fileChange.type === 'patch' + ? 'String replace applied successfully.' + : 'Wrote file successfully.' for (const file of created) { results.push({ file, - message: 'Created new file', - unifiedDiff: lines.join('\n'), + message: successMessage, }) } for (const file of modified) { results.push({ file, - message: 'Updated file', - unifiedDiff: lines.join('\n'), + message: successMessage, }) } @@ -73,7 +73,7 @@ export async function changeFile(params: { results.push({ file, errorMessage: `Failed to apply patch.`, - patch: lines.join('\n'), + patch: fileChange.content, }) } From 9837c2a88dbcffd3d4042c058ce0110498b54169 Mon Sep 17 00:00:00 2001 From: James Grugett Date: Tue, 5 May 2026 12:11:37 -0700 Subject: [PATCH 2/7] Clarify write_file success messages --- sdk/src/__tests__/change-file.test.ts | 35 +++++++++++++++++++++++++-- sdk/src/tools/change-file.ts | 14 ++++++----- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/sdk/src/__tests__/change-file.test.ts b/sdk/src/__tests__/change-file.test.ts index 9c50ed089e..dff8969c7e 100644 --- a/sdk/src/__tests__/change-file.test.ts +++ b/sdk/src/__tests__/change-file.test.ts @@ -36,7 +36,7 @@ describe('changeFile', () => { ) }) - test('returns a simple success message for file writes', async () => { + test('returns a simple success message for new file writes', async () => { const fs = createMockFs() const result = await changeFile({ @@ -54,7 +54,7 @@ describe('changeFile', () => { type: 'json', value: { file: 'src/file.ts', - message: 'Wrote file successfully.', + message: 'Created file successfully.', }, }, ]) @@ -62,4 +62,35 @@ describe('changeFile', () => { '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 12c5ddb0fb..ff34cc547a 100644 --- a/sdk/src/tools/change-file.ts +++ b/sdk/src/tools/change-file.ts @@ -50,22 +50,24 @@ export async function changeFile(params: { }) const results: CodebuffToolOutput<'str_replace'>[0]['value'][] = [] - const successMessage = - fileChange.type === 'patch' - ? 'String replace applied successfully.' - : 'Wrote file successfully.' for (const file of created) { results.push({ file, - message: successMessage, + message: + fileChange.type === 'patch' + ? 'String replace applied successfully.' + : 'Created file successfully.', }) } for (const file of modified) { results.push({ file, - message: successMessage, + message: + fileChange.type === 'patch' + ? 'String replace applied successfully.' + : 'Overwrote file successfully.', }) } From e7f1c593d6da42685cafad66cb8ca865d00a7992 Mon Sep 17 00:00:00 2001 From: James Grugett Date: Tue, 5 May 2026 13:40:56 -0700 Subject: [PATCH 3/7] Sync agent tool replacement types --- agents/types/tools.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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 }[] } From 28b9daae830271adefa4e8470f8528d5500eb2a2 Mon Sep 17 00:00:00 2001 From: James Grugett Date: Tue, 5 May 2026 13:45:41 -0700 Subject: [PATCH 4/7] Fix create result handling --- .agents/types/tools.ts | 16 ++++++++-------- cli/src/components/tools/str-replace.tsx | 6 +++++- .../utils/__tests__/implementor-helpers.test.ts | 11 +++++++++++ cli/src/utils/implementor-helpers.ts | 3 ++- 4 files changed, 26 insertions(+), 10 deletions(-) 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/cli/src/components/tools/str-replace.tsx b/cli/src/components/tools/str-replace.tsx index 4391996713..5ecae0fd8a 100644 --- a/cli/src/components/tools/str-replace.tsx +++ b/cli/src/components/tools/str-replace.tsx @@ -40,6 +40,10 @@ function extractValueForKey(output: string, key: string): string | null { return null } +function isCreatedFileMessage(message: string | null): boolean { + return message === 'Created file successfully.' || message === 'Created new file' +} + interface EditHeaderProps { name: string filePath: string | null @@ -97,7 +101,7 @@ export const StrReplaceComponent = defineToolComponent({ ? (toolBlock.input as any).path : null) const message = extractValueForKey(outputStr, 'message') - const isCreate = message === 'Created new file' + const isCreate = isCreatedFileMessage(message) return { content: ( diff --git a/cli/src/utils/__tests__/implementor-helpers.test.ts b/cli/src/utils/__tests__/implementor-helpers.test.ts index 0f49973495..d04ed46001 100644 --- a/cli/src/utils/__tests__/implementor-helpers.test.ts +++ b/cli/src/utils/__tests__/implementor-helpers.test.ts @@ -215,6 +215,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', diff --git a/cli/src/utils/implementor-helpers.ts b/cli/src/utils/implementor-helpers.ts index 996bb6957c..9dc3b9d8b5 100644 --- a/cli/src/utils/implementor-helpers.ts +++ b/cli/src/utils/implementor-helpers.ts @@ -325,7 +325,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')) ) } From 1bb1113a6dc1bdb30a689894e6aac2731ada7d57 Mon Sep 17 00:00:00 2001 From: James Grugett Date: Tue, 5 May 2026 14:14:21 -0700 Subject: [PATCH 5/7] Render edit diffs from tool input --- cli/src/components/tools/str-replace.tsx | 64 +++---------------- .../__tests__/implementor-helpers.test.ts | 38 +++++++++++ 2 files changed, 48 insertions(+), 54 deletions(-) diff --git a/cli/src/components/tools/str-replace.tsx b/cli/src/components/tools/str-replace.tsx index 5ecae0fd8a..8c045e52c0 100644 --- a/cli/src/components/tools/str-replace.tsx +++ b/cli/src/components/tools/str-replace.tsx @@ -3,47 +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 -} - -function isCreatedFileMessage(message: string | null): boolean { - return message === 'Created file successfully.' || message === 'Created new file' -} - interface EditHeaderProps { name: string filePath: string | null @@ -70,14 +37,13 @@ interface EditBodyProps { name: string filePath: string | null diffText: string - isCreate: boolean } -const EditBody = ({ name, filePath, diffText, isCreate }: EditBodyProps) => { +const EditBody = ({ name, filePath, diffText }: EditBodyProps) => { return ( - {!isCreate && diffText.length > 0 && ( + {diffText.length > 0 && ( @@ -90,18 +56,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 = isCreatedFileMessage(message) + const diff = extractDiff(toolBlock) + const filePath = extractFilePath(toolBlock) + const isCreate = isCreateFile(toolBlock) return { content: ( @@ -109,7 +66,6 @@ export const StrReplaceComponent = defineToolComponent({ name={isCreate ? 'Create' : 'Edit'} filePath={filePath} diffText={diff ?? ''} - isCreate={isCreate} /> ), } diff --git a/cli/src/utils/__tests__/implementor-helpers.test.ts b/cli/src/utils/__tests__/implementor-helpers.test.ts index d04ed46001..f7e0daee1c 100644 --- a/cli/src/utils/__tests__/implementor-helpers.test.ts +++ b/cli/src/utils/__tests__/implementor-helpers.test.ts @@ -117,6 +117,32 @@ 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('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('constructs diff from write_file input', () => { const block: ToolContentBlock = { type: 'tool', @@ -128,6 +154,18 @@ 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('constructs diff from propose_str_replace input', () => { const block: ToolContentBlock = { type: 'tool', From 07402e80db41d7b5d5c25d5b6dc8c43f207c58f3 Mon Sep 17 00:00:00 2001 From: James Grugett Date: Tue, 5 May 2026 14:18:28 -0700 Subject: [PATCH 6/7] Hide diffs for created files --- cli/src/components/tools/str-replace.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cli/src/components/tools/str-replace.tsx b/cli/src/components/tools/str-replace.tsx index 8c045e52c0..10e00672cf 100644 --- a/cli/src/components/tools/str-replace.tsx +++ b/cli/src/components/tools/str-replace.tsx @@ -37,13 +37,14 @@ interface EditBodyProps { name: string filePath: string | null diffText: string + isCreate: boolean } -const EditBody = ({ name, filePath, diffText }: EditBodyProps) => { +const EditBody = ({ name, filePath, diffText, isCreate }: EditBodyProps) => { return ( - {diffText.length > 0 && ( + {!isCreate && diffText.length > 0 && ( @@ -66,6 +67,7 @@ export const StrReplaceComponent = defineToolComponent({ name={isCreate ? 'Create' : 'Edit'} filePath={filePath} diffText={diff ?? ''} + isCreate={isCreate} /> ), } From 75f7fa905fb3a8e134fd8b7e536e2f3071b19baa Mon Sep 17 00:00:00 2001 From: James Grugett Date: Tue, 5 May 2026 14:38:14 -0700 Subject: [PATCH 7/7] Skip failed edit previews --- .../__tests__/implementor-helpers.test.ts | 99 +++++++++++++++++ cli/src/utils/implementor-helpers.ts | 105 +++++++++++++++++- 2 files changed, 203 insertions(+), 1 deletion(-) diff --git a/cli/src/utils/__tests__/implementor-helpers.test.ts b/cli/src/utils/__tests__/implementor-helpers.test.ts index f7e0daee1c..03699fc41c 100644 --- a/cli/src/utils/__tests__/implementor-helpers.test.ts +++ b/cli/src/utils/__tests__/implementor-helpers.test.ts @@ -132,6 +132,24 @@ describe('extractDiff', () => { 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', @@ -143,6 +161,38 @@ describe('extractDiff', () => { 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', @@ -166,6 +216,17 @@ describe('extractDiff', () => { 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', @@ -367,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', () => { @@ -414,6 +494,25 @@ 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', () => { diff --git a/cli/src/utils/implementor-helpers.ts b/cli/src/utils/implementor-helpers.ts index 9dc3b9d8b5..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,32 +233,55 @@ 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) @@ -271,6 +306,70 @@ 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. */ @@ -425,6 +524,8 @@ export function getFileStatsFromBlocks( block.toolName as (typeof ALL_EDIT_TOOL_NAMES)[number], ) ) { + if (isFailedEditToolBlock(block)) continue + const filePath = extractFilePath(block) if (!filePath) continue @@ -475,6 +576,8 @@ export function buildActivityTimeline( 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)