ui: add read_media tool - #25877
Conversation
|
Hi @parabelboi, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
allozaur
left a comment
There was a problem hiding this comment.
few architectural remarks. but i really like it :) great stuff. @ngxson @ServeurpersoCom u guys can do a more thorough review/testing on the server side + security aspects
| const extras = section.toolResultExtras; | ||
| if (!extras || extras.length === 0) return null; | ||
| // Extract the attachment name from the cleaned result text | ||
| const match = section.toolResult?.match(/\[Attachment saved: ([^\]]+)\]/); |
There was a problem hiding this comment.
let's move the regex to constants
| if (trimmed.startsWith('Image: ')) { | ||
| path = trimmed.slice('Image: '.length).trim(); | ||
| fileName = path.split('/').pop() ?? path; | ||
| } else if (trimmed.startsWith('Size: ')) { | ||
| const match = trimmed.match(/Size:\s*(\d+)\s*bytes/); | ||
| if (match) sizeBytes = parseInt(match[1], 10); | ||
| } else if (trimmed.startsWith('MIME: ')) { | ||
| mimeType = trimmed.slice('MIME: '.length).trim(); |
There was a problem hiding this comment.
magic strings/regexes — let's move to $lib/constants
| export function parseReadImageMeta(section: AgenticSection): ReadImageMeta | null { | ||
| if (!section.toolResult) return null; | ||
|
|
||
| const lines = section.toolResult.split('\n'); |
There was a problem hiding this comment.
should use existing NEWLINE constant
ngxson
left a comment
There was a problem hiding this comment.
I don't think it should be a dedicated tool. Instead, it should be read_file tool with a parameter type:
type: can be"text"or"media"; if "media", returns file content as base64 with mimetype; media file can be image, audio or video file
I also thought about this, but decided to have a separate tool, because for two reasons: First (technical) point is, that users can decide to disable Second (non-technical) point was, that it might be quicker to integrate a new tool, than to extend an existing one. It would create less friction because of separation of concerns. |
| return (it != mime_map.end()) ? it->second : "application/octet-stream"; | ||
| } | ||
|
|
||
| struct server_tool_read_image : server_tool { |
There was a problem hiding this comment.
first, I'd expect to have a generic read_media tool that allow reading image/audio/video
second, if we expect to keep read_media / read_file as separate tools, I'd expect server_tool_read_media to be a derived class of server_tool_read_file
the read_media::invoke() should do only one job: convert the output to base64 and provide the appropriate mime type
There was a problem hiding this comment.
Would it be sufficient if the server_tool_read_media class is a derived class of server_tool_read_file, but would only support images (for now)?
There was a problem hiding this comment.
And do you still want to have a media-type parameter and if so, would it be ok, if it's optional? Because making it mandatory and defining media-specific options based on media-type would make the json-schema more complicated.
There was a problem hiding this comment.
hmm I think it's ok to allow image-only server_tool_read_media for now, maybe we should add a param type that can be image/audio/video in the future, tbd
There was a problem hiding this comment.
Very good, I'll rename the tool and leave the type parameter out for now.
Also I'd concentrate to only a single file for now:
I already have a branch that reads in multiple images successfully, but that required a small fix in agentic.svelte.ts (directly after the tool call, attachments have been displayed duplicated, after a page reload everything was fine again). That could lead to a regression in mcp tools and needsmore testing especially with multiple attachments. Also it's way more code to review and test. Especially the code that collects the list of files based on a glob expression still contains bugs. So it's probably a good idea to leave that code out for now.
hmm ok maybe better to skip video for now, as the file can be large. but audio might still be a valid use case for video, we might be better to allow /v1/chat/completion to read the file directly somehow, but I need to think more about it because that can introduce a huge security risk. the tools system doesn't apply because it's already allowed to read arbitrary files by default. |
| } | ||
|
|
||
| // | ||
| // read_media: read a media file (image or audio) and return base64-encoded data with metadata |
There was a problem hiding this comment.
this is not the good placement in the file, move it to after the last tool definition above
There was a problem hiding this comment.
yes, indeed. section has been moved
|
@ggml-gh-bot review |
Automated code reviewReview of PR #25877 - I ran the always-on scope/security/general checks plus the Server checklist (touched paths are Blocking(point 1) Audio attachments are stored with the wrong field, breaking the type contract, model input, and UI rendering. { type: AttachmentType.AUDIO, name, mimeType, base64Url: trimmedLine }But
Decide on one shape and apply it consistently: either push (point 2) Server advertises audio formats the model-input path cannot handle. Will slow the review(point 3) (point 4) No test for the new builtin tool. Nits(point 5) Stray duplicated (point 6) Svelte indentation in (point 7) (point 8) Overall the metadata/parsing wiring (prefix constants on both sides, This review was generated automatically by pi coding agent using |
|
please address point 1, 2, 3; testing is optional, but recommended to add |
I already tried several models with audio support, but did not get it working. I guess now I know why ;-) |
23ee634 to
2ac20c5
Compare
Hi ngxson, I addressed all three points and rebased the branch to current master. Audio input also now works, even if the model does not support audio: I'll also (try to) have a look into the remaining comments (especially the "missing testcase" one). |
|
/bot review |
You are right. I think I introduced a bug with the rebase: I just tested with an audio capable model, and audio input does not work anymore. I'll have a look. |
ngxson
left a comment
There was a problem hiding this comment.
@parabelboi sorry I still need to rethink this tool, I'm not convinced that it should be a server tool.
the problem is that for all other tools, the frontend simply forward the request and response between model <--> backend without any transformations. however, this is the important part: the read_media tool is not in this category, the response must be preprocess to put the base64 into the correct field of the message, it is not "just another raw json" response
in addition: frontend must switch the tool based on model's capability, backend doesn't do this. so an always-enabled read_media make no sense as it will be toggled by the frontend anyway.
so what I expect is something like this instead:
- no new server tool, just extend the current
read_filetool to return base64 instead (via a new headerX-Resp-Type, not a param, so that it's not exposed to the LLM) read_mediawill become a synthetic tool provided by the frontend
this keep the backend tool as pure i/o primitive. please follow this plan instead.
@ngxson, I am sorry to hear that. Three weeks ago I opened an issue regarding a read_image tool and proposed a design and a draft implementation. You asked me to broaden the scope, I invested some more time and extended it. We went through some review cycles and I invested some more time to polish it. And now (two one-liners before the goal) you are asking me to go back to the start? And what if in three weeks later you (or another maintainer) would be asking me to follow yet another path? |
|
unless you can prove it otherwise, I want to point out the obvious here:
edit: sorry for being a bit harsh here, but I want to remind that this is not the only PR that I had the reviewed today. I appreciate your time & efforts, but technical requirements is till technical requirements, we cannot merge a PR that will be replaced right after it's merged if you really don't want to continue with this PR - just tell us - I can take over it & push changes directly here if you like |
|
Thanks for pushing this as far as you did, and sorry the review loop cost you so many evenings. Design churn on a tool we will have to live with for a long time is not a judgment on your work, it is just how these things settle, and with today's AI tooling a prototype is cheap enough that throwing one away is a normal step rather than a loss. If you would rather stop here, that is completely fine: once the design is settled with ngxson I can rewrite it from scratch in a few hours, and that is only possible because your attempt mapped out the paths that do not work. The feature is wanted, so it will land either way. |
@ngxson: |
I think that would be good. |
if I knew about it beforehand but intentionally held off and only told you afterwards, that would have been 100% my fault. but the reality: I only thought about it today. your blaming is wrong here that being said, I will take over this PR as you confirmed above |
Sorry, I did not want to imply intentional holding off. Let me rephrase it: |
|
can be merged after @allozaur or @ServeurpersoCom review the UI part |
... also good on my side. And thanks for taking over. This is now of course the better solution. I was just a bit frustrated about the wasted time. Well not a total waste, because I learnt something of course ;-) |
|
With bated breath, I await this beast to be released! |
allozaur
left a comment
There was a problem hiding this comment.
Few architectural nits. Let's address them and then we good to go
Replace the magic strings, regexes and number in the read_media parser and service with named constants. Path splitting reuses FILE_PATH_SEPARATOR_REGEX, the size header regex moves to READ_MEDIA_SIZE_REGEX derived from PREFIX_SIZE, and FILE_EXTENSION_SEPARATOR lands next to it in constants/code.ts.
|
@ngxson GH blocks merging unless u approve as u requested changes earlier |
|
Noticed a few minor (display) issues while reviewing, nothing blocking. Will follow up in a separate PR :) |
Signed-off-by: Gabe Goodhart <ghart@us.ibm.com> * origin/master: (383 commits) cmake : introduce semantic versioning (ggml-org#26839) gguf : harden loader against malformed tensor dims and metadata types (ggml-org#25596) kleidiai: Add runtime feature detection mechanism for aarch64/kleidiai (ggml-org#26076) model : disallow integer dflash sliding_window_pattern (ggml-org#26900) sync : ggml cmake : add config version support (ggml/1582) server : support slot save/restore with media inputs (ggml-org#26640) ui: add read_media tool (ggml-org#25877) opencl: default FA c8 cluster width to 16 on X1E (ggml-org#26433) tests : update speculative params (ggml-org#26925) vulkan: add TQ2_0 (ternary) support (ggml-org#25850) wavtokenizer-dec : bound posnet/convnext block_count against n_layer_all (ggml-org#26892) convert : handle per_layer_config in Gemma4 (transformers 5.15) (ggml-org#26882) opencl: use flat mv q5_k when weight exceeds image1d_buffer_t limit (ggml-org#26880) chat : fix muse-glimmer detection of tool calls after EOM (ggml-org#26879) ci : add missing release check (ggml-org#26923) CUDA: only disable CUDA graphs when mul_mat_id actually needs a stream sync (ggml-org#26802) cuda : add warp-per-row wkv7 kernel for single-token decode (ggml-org#26111) spec : update speculative-simple (ggml-org#26904) chat : tighten bare function parsing for Qwen models (ggml-org#26793) ...
* server: add read_image tool (ggml-org#25875) Adds a server-tool that allows vision models to analyze server-side images. This tool is reading a single file for now: The image data is base64 encoded and passed to the UI, which decodes it, fills the <img> tag and removes the data URI before passing the tool result back to the model. * cleanup read_image tool: move magic strings to constants * Add dedicated constants file: tools/ui/src/lib/constants/read-image.ts with PREFIX_IMAGE, PREFIX_SIZE, PREFIX_MIME constants * Use ATTACHMENT_SAVED_REGEX from agentic.ts in ChatMessageToolCallBlockReadImage.svelte * Use NEWLINE constant from code.ts instead of hardcoded '\n' * Use PREFIX_SIZE in regex pattern for size parsing * Add SERVER_TOOL_READ_IMAGE_PREFIX_* constants in C++ server-tools.cpp to match the TypeScript PREFIX_* constants for consistency * server: rename read_image tool to read_media for images and audio * Rename server_tool_read_image to server_tool_read_media in C++ * Rename enum BuiltInTool.READ_IMAGE to READ_MEDIA * Rename UI constants, parser, and Svelte component files * Update display label from 'Read image' to 'Read media' * ui: consolidate audio data URI handling into shared utility * Extract getAudioInputFormat to a shared utility (was duplicated inline) * Store raw base64 in base64Data on the message object * Use base64Data to construct data URIs for audio rendering * Update agentic store to build INPUT_AUDIO parts from base64Data * server: read_media: restrict audio to wav/mp3 and minor fixes * Server get_mime_from_extension now only advertises audio/wav and audio/mpeg (the only formats the model's input_audio API accepts) * Case-insensitive extension matching (fixes .MP3, .Wav, etc.) * Unknown extensions return an error instead of a multi-MB data URI that inflates model context with garbage * Updated tool description to document supported formats * Frontend AUDIO_MIME_TO_EXTENSION trimmed to match server * fix a missing import in tools/ui/src/lib/stores/agentic.svelte.ts * server: read_media: add to --tools help text and README tool list * ui: fix indentation in ChatMessageToolCallBlockDefault.svelte * server: read_media tool: fix a cast to use the correct type * server: read_media: multiple fixes * server-tools.cpp import cctype, remove UTF-8 char, check mime before reading file * ui: add MimeTypePrefix.AUDIO and use it in agentic.svelte.ts * server: make read_media inherit from read_file and add uses_cwd * ui: fix formating issues * rm from server * move it to frontend-only tool * correct partial commit * rm unused * ui: address review from allozaur Replace the magic strings, regexes and number in the read_media parser and service with named constants. Path splitting reuses FILE_PATH_SEPARATOR_REGEX, the size header regex moves to READ_MEDIA_SIZE_REGEX derived from PREFIX_SIZE, and FILE_EXTENSION_SEPARATOR lands next to it in constants/code.ts. --------- Co-authored-by: ckrafft <ckrafft@epyc> Co-authored-by: Xuan Son Nguyen <son@huggingface.co> Co-authored-by: Pascal <admin@serveurperso.com>








This adds a server-tool that allows vision models to analyze server-side images. It fixes #25875
Overview
This tool is reading only a single file for now:
The image data is base64 encoded and passed to the UI, which decodes it, fills the
<img>tag and removes the data URI before passing the tool result back to the model.Additional information
Requirements