Skip to content

fix: enable image file reading in read tool (fixes #451) #568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

morgvanny
Copy link

The read tool was throwing an error for image files instead of reading them. This fix implements proper image handling by converting images to base64 data URLs, allowing opencode to read PNG, JPEG, and other image formats as documented. Should fix #451

The read tool was throwing an error for image files instead of reading them.
This fix implements proper image handling by converting images to base64 data URLs,
allowing opencode to read PNG, JPEG, and other image formats as documented.
Copy link

@delorenj delorenj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Enable Image File Reading in Read Tool

Overview

This PR fixes issue #451 by implementing proper image handling in the read tool. Instead of throwing an error when encountering image files, it now converts them to base64 data URLs, allowing opencode to properly read PNG, JPEG, and other image formats.

Code Quality Analysis

Strengths:

  • Clean implementation that maintains existing code structure
  • Proper MIME type handling for different image formats
  • Follows existing patterns (LSP touchFile, FileTime.read)
  • Comprehensive test coverage with real image data

Implementation Details:

  • Converts image files to base64 data URLs wrapped in <image> tags
  • Supports PNG, JPEG, GIF, BMP, SVG, and WebP formats
  • Maintains consistent metadata format with text files

Suggestions for Improvement

  1. Memory Consideration: Loading entire images into memory via arrayBuffer() could be problematic for large files. Consider adding a size check:

    const stats = await file.stat()
    if (stats.size > MAX_IMAGE_SIZE) {
      throw new Error(`Image file too large: ${stats.size} bytes`)
    }
  2. Error Handling: Add try-catch for buffer operations to handle corrupted image files gracefully

  3. MIME Type Enhancement: The getMimeType function could use the actual file content for validation rather than just extension

Test Coverage

  • Excellent test coverage with real binary data for PNG and JPEG
  • Tests verify correct data URL format and metadata
  • Includes error case for non-existent files
  • Consider adding tests for edge cases (corrupted files, very large files)

Security & Performance

  • Security: Base64 encoding is safe for image display
  • Performance: Current implementation is efficient for reasonable image sizes
  • Risk: No file size validation could lead to memory issues with large images

Overall Assessment

This is a solid fix that properly addresses the issue. The code is clean, well-tested, and follows project conventions. With the addition of file size validation, this PR would be ready to merge.

@thdxr thdxr self-assigned this Jul 2, 2025
@morgvanny
Copy link
Author

morgvanny commented Jul 2, 2025

It looks like there are server side validations enforcing a 5mb limit and a 2000px limit. It should handle if it's corrupt as well. I have some code I could push to validate client-side if we'd like to provide immediate feedback without an API request/network round trip. The trade-off is additional complexity and the need to maintain these limits in sync with the API. Happy to push the client-side validation or keep the simpler approach.

@morgvanny morgvanny closed this Jul 15, 2025
@adamdotdevin
Copy link
Collaborator

i think rather than checking against limits that may change we want to just do a resize and compress step for all image uploads greater than some threshold (or maybe just all images). lmk if you want to take a stab at that. should be done on the TS server side so that other clients benefit as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing support for PNG images?
4 participants