Skip to content

Improve attachment handling#7

Merged
khasinski merged 1 commit intokhasinski:mainfrom
9elements:main
Apr 6, 2026
Merged

Improve attachment handling#7
khasinski merged 1 commit intokhasinski:mainfrom
9elements:main

Conversation

@molily
Copy link
Copy Markdown
Contributor

@molily molily commented Mar 30, 2026

Hi, first of all, thank you for this gem!

In the standard OpenAI provider (old API), ruby_llm …

  1. raises an exception on unknown/unsupported attachment types
  2. embeds the content of text files (.txt, .csv etc.)
  3. concentrates the attachment handling in media.rb

This gem in contrast …

  1. adds { type: 'input_text', text: "[Attachment: #{attachment.type}]" } for any unsupported attachment.
    => According to our tests, this only confuses the LLM since it does not have access to the file content. It's better to refuse or ignore the attachment at all.
  2. does not embed the content of text files. It handles them as an unsupported attachment (see above).
    => We'd like to send .text, .csv, .html etc. to the LLM, so the standard ruby_llm behavior makes sense to us.
  3. duplicates the attachment handling in media.rb and and chat.rb (format_attachment methods).

So my proposal is that this provider should behave the same as the ruby_llm standard OpenAI provider regarding these three points.

What do you think? Thanks for considering this!

Regarding the files in fixtures: They are taken from the ruby_llm gem.

Another related question: Is Media#format_content called anywhere? As far as I can see, not from within this gem, also not from ruby_llm if I'm not mistaken. Then we could remove this method since it is very similar to Chat#format_content.

- Support text files
- Support xlsx files
- Throw an UnsupportedAttachmentError instead of attaching a pointless empty attachment
- Reuse the format_attachment logic from Media in Chat like ruby_llm does
@khasinski khasinski merged commit ac3ea53 into khasinski:main Apr 6, 2026
5 checks passed
@khasinski
Copy link
Copy Markdown
Owner

khasinski commented Apr 6, 2026

Thanks @molily for this clean improvement! Merged. I've verified all three scenarios work end-to-end with the live API (text files, images, unsupported type rejection). As a follow-up, I'll remove the dead Media#format_content method you flagged - you're right that it's unused by this provider.

khasinski added a commit that referenced this pull request Apr 6, 2026
- Text file support, raise on unsupported types, deduplicate Media logic
  (PR #7 by @molily)
- Remove dead Media#format_content method
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.

2 participants