Skip failed edit previews

Author: jahooma

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', () => {

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<string, unknown>
+    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<string, unknown>
+    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<string, unknown>
   const baseToolName = getBaseToolName(toolBlock.toolName)
 
@@ -271,6 +306,70 @@ export function extractDiff(toolBlock: ToolContentBlock): string | null {
   return null
 }
 
+function hasErrorMessage(value: Record<string, unknown>): 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<string, unknown>
+    if (hasErrorMessage(value)) return true
+  }
+  if (typeof outputRaw === 'object' && outputRaw !== null) {
+    const rawObj = outputRaw as Record<string, unknown>
+    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)