Fix image sending: use pre-processed data instead of re-reading from disk

brandonkachen · brandonkachen · commit 41c91d0546c5 · 2026-01-26T15:52:16.000-08:00
Images attached via clipboard or file path were being re-processed from
disk when sending messages, causing failures when paths resolved
differently across repos. Now uses the already-processed base64 data
stored in pendingImages, only calling processImageFile for inline
image paths detected in message content.
diff --git a/cli/src/utils/__tests__/image-processor.test.ts b/cli/src/utils/__tests__/image-processor.test.ts
@@ -4,21 +4,25 @@ import { processImagesForMessage } from '../image-processor'
 
 import type { PendingImageAttachment } from '../../state/chat-store'
 
-const createPendingImage = (path: string): PendingImageAttachment => ({
+const createPendingImage = (path: string, processedImage?: { base64: string; mediaType: string }): PendingImageAttachment => ({
   kind: 'image',
   path,
   filename: path.split('/').pop() ?? 'image.png',
   status: 'ready',
+  ...(processedImage && { processedImage }),
 })
 
 describe('processImagesForMessage', () => {
-  test('deduplicates image paths and returns message content', async () => {
-    const pendingImages = [createPendingImage('/tmp/pic.png')]
+  test('uses pre-processed image data from pendingImages without re-reading from disk', async () => {
+    const pendingImages = [createPendingImage('/tmp/pic.png', {
+      base64: 'pre-processed-base64-data',
+      mediaType: 'image/png',
+    })]
     const processor = mock(async () => ({
       success: true,
       imagePart: {
         type: 'image' as const,
-        image: 'base64-data',
+        image: 'disk-base64-data',
         mediaType: 'image/png',
       },
     }))
@@ -30,24 +34,123 @@ describe('processImagesForMessage', () => {
       processor: processor as any,
     })
 
-    expect(processor).toHaveBeenCalledTimes(1)
+    // Should NOT call processor since we have pre-processed data
+    expect(processor).not.toHaveBeenCalled()
     expect(result.attachments).toHaveLength(1)
     expect(result.messageContent?.[0]).toMatchObject({
       type: 'image',
-      image: 'base64-data',
+      image: 'pre-processed-base64-data',
+      mediaType: 'image/png',
+    })
+  })
+
+  test('processes inline image paths that are not in pendingImages', async () => {
+    const pendingImages = [createPendingImage('/tmp/pic.png', {
+      base64: 'pre-processed-base64-data',
+      mediaType: 'image/png',
+    })]
+    const processor = mock(async () => ({
+      success: true,
+      imagePart: {
+        type: 'image' as const,
+        image: 'inline-base64-data',
+        mediaType: 'image/jpeg',
+      },
+    }))
+
+    const result = await processImagesForMessage({
+      content: 'Here is another image @/tmp/other.jpg',
+      pendingImages,
+      projectRoot: '/repo',
+      processor: processor as any,
+    })
+
+    // Should call processor only for the inline path
+    expect(processor).toHaveBeenCalledTimes(1)
+    expect(processor).toHaveBeenCalledWith('/tmp/other.jpg', '/repo')
+    expect(result.messageContent).toHaveLength(2)
+    expect(result.messageContent?.[0]).toMatchObject({
+      type: 'image',
+      image: 'pre-processed-base64-data',
+    })
+    expect(result.messageContent?.[1]).toMatchObject({
+      type: 'image',
+      image: 'inline-base64-data',
+    })
+  })
+
+  test('backwards compatibility: processes from disk when processedImage is missing', async () => {
+    // This tests the edge case where processedImage is missing but status is 'ready'
+    const pendingImages = [createPendingImage('/tmp/pic.png')] // No processedImage
+    const warn = mock(() => {})
+    const processor = mock(async () => ({
+      success: true,
+      imagePart: {
+        type: 'image' as const,
+        image: 'disk-base64-data',
+        mediaType: 'image/png',
+      },
+    }))
+
+    const result = await processImagesForMessage({
+      content: '',
+      pendingImages,
+      projectRoot: '/repo',
+      processor: processor as any,
+      log: { warn } as any,
+    })
+
+    // Should warn about missing processedImage and fall back to disk
+    expect(warn).toHaveBeenCalled()
+    expect(processor).toHaveBeenCalledTimes(1)
+    expect(result.messageContent?.[0]).toMatchObject({
+      type: 'image',
+      image: 'disk-base64-data',
     })
   })
 
-  test('logs warnings when processing fails', async () => {
+  test('skips images with processing or error status', async () => {
+    const pendingImages: PendingImageAttachment[] = [
+      { kind: 'image', path: '/tmp/processing.png', filename: 'processing.png', status: 'processing' },
+      { kind: 'image', path: '/tmp/error.png', filename: 'error.png', status: 'error', note: 'failed' },
+      createPendingImage('/tmp/ready.png', { base64: 'ready-data', mediaType: 'image/png' }),
+    ]
+    const processor = mock(async () => ({
+      success: true,
+      imagePart: {
+        type: 'image' as const,
+        image: 'should-not-be-used',
+        mediaType: 'image/png',
+      },
+    }))
+
+    const result = await processImagesForMessage({
+      content: '',
+      pendingImages,
+      projectRoot: '/repo',
+      processor: processor as any,
+    })
+
+    // Should not call processor at all (ready image has processedImage)
+    expect(processor).not.toHaveBeenCalled()
+    // Only the ready image should be in messageContent
+    expect(result.messageContent).toHaveLength(1)
+    expect(result.messageContent?.[0]).toMatchObject({
+      type: 'image',
+      image: 'ready-data',
+    })
+  })
+
+  test('logs warnings when inline path processing fails', async () => {
     const warn = mock(() => {})
-    const pendingImages = [createPendingImage('/tmp/fail.png')]
+    const pendingImages: PendingImageAttachment[] = []
     const processor = mock(async () => ({
       success: false,
       error: 'boom',
     }))
 
     const result = await processImagesForMessage({
-      content: '',
+      content: 'Here is an image @/tmp/fail.png',
       pendingImages,
       projectRoot: '/repo',
       processor: processor as any,
@@ -57,4 +160,35 @@ describe('processImagesForMessage', () => {
     expect(warn).toHaveBeenCalled()
     expect(result.messageContent).toBeUndefined()
   })
+
+  test('deduplicates: does not process inline path that matches pending image path', async () => {
+    const pendingImages = [createPendingImage('/tmp/pic.png', {
+      base64: 'pre-processed-data',
+      mediaType: 'image/png',
+    })]
+    const processor = mock(async () => ({
+      success: true,
+      imagePart: {
+        type: 'image' as const,
+        image: 'disk-data',
+        mediaType: 'image/png',
+      },
+    }))
+
+    const result = await processImagesForMessage({
+      content: 'Here is the same image @/tmp/pic.png and again /tmp/pic.png',
+      pendingImages,
+      projectRoot: '/repo',
+      processor: processor as any,
+    })
+
+    // Should not call processor since the path is already in pendingImages
+    expect(processor).not.toHaveBeenCalled()
+    // Should only have one image in messageContent (no duplicates)
+    expect(result.messageContent).toHaveLength(1)
+    expect(result.messageContent?.[0]).toMatchObject({
+      type: 'image',
+      image: 'pre-processed-data',
+    })
+  })
 })
diff --git a/cli/src/utils/image-processor.ts b/cli/src/utils/image-processor.ts
@@ -34,21 +34,69 @@ export const processImagesForMessage = async (params: {
     log = logger,
   } = params
 
-  const detectedImagePaths = extractImagePaths(content)
-  const allImagePaths = [
-    ...pendingImages.map((img) => img.path),
-    ...detectedImagePaths,
-  ]
-  const uniqueImagePaths = [...new Set(allImagePaths)]
-
   const attachments = pendingImages.map((img) => ({
     path: img.path,
     filename: img.filename,
     size: img.size,
   }))
 
   const validImageParts: ProcessedImagePart[] = []
-  for (const imagePath of uniqueImagePaths) {
+
+  // First, use pre-processed data from pendingImages (already processed when attached)
+  // This avoids re-reading from disk, which can fail if the path is relative to a different cwd
+  const pendingImagePaths = new Set<string>()
+  for (const img of pendingImages) {
+    pendingImagePaths.add(img.path)
+
+    if (img.processedImage) {
+      // Use the already-processed image data
+      validImageParts.push({
+        type: 'image',
+        image: img.processedImage.base64,
+        mediaType: img.processedImage.mediaType,
+        filename: img.filename,
+        size: img.size,
+        width: img.width,
+        height: img.height,
+        path: img.path,
+      })
+    } else if (img.status === 'ready') {
+      // Backwards compatibility: if processedImage is missing but status is ready,
+      // try to process from disk (shouldn't happen in normal flow)
+      log.warn(
+        { imagePath: img.path },
+        'Pending image marked ready but missing processedImage data, re-processing from disk',
+      )
+      const result = await processor(img.path, projectRoot)
+      if (result.success && result.imagePart) {
+        validImageParts.push({
+          type: 'image',
+          image: result.imagePart.image,
+          mediaType: result.imagePart.mediaType,
+          filename: result.imagePart.filename,
+          size: result.imagePart.size,
+          width: result.imagePart.width,
+          height: result.imagePart.height,
+          path: img.path,
+        })
+      } else if (!result.success) {
+        log.warn(
+          { imagePath: img.path, error: result.error },
+          'Failed to process pending image from disk',
+        )
+      }
+    }
+    // Skip images with status 'processing' or 'error' - they shouldn't be sent
+  }
+
+  // Then, process any inline image paths from the message content that aren't already in pendingImages
+  const detectedImagePaths = extractImagePaths(content)
+  for (const imagePath of detectedImagePaths) {
+    // Skip if this path is already handled by pendingImages
+    if (pendingImagePaths.has(imagePath)) {
+      continue
+    }
+
     const result = await processor(imagePath, projectRoot)
     if (result.success && result.imagePart) {
       validImageParts.push({
@@ -64,7 +112,7 @@ export const processImagesForMessage = async (params: {
     } else if (!result.success) {
       log.warn(
         { imagePath, error: result.error },
-        'Failed to process image for SDK',
+        'Failed to process inline image path for SDK',
       )
     }
   }
