From cfaa7bace3961ab050e08e2a3a93b6c77a976cc4 Mon Sep 17 00:00:00 2001 From: xavix-yo Date: Wed, 22 Jul 2026 11:01:44 +0100 Subject: [PATCH] feat(read_file): let capable models view images, video and PDFs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the resolved model declares an input modality (vision → images, video, document → PDFs), read_file now hands a binary media file back to the model as native input instead of failing on non-UTF-8 bytes. - ToolResult gains a Media { text, media } variant carrying MediaRef { host_path, mime }; the tool message keeps only the text note, the bytes travel out of band in a new additive chat_llm_tools.media column (mirrors preview_old/new). - read_file sniffs the resolved host file; a recognized medium becomes a Media result (with a neutral note), everything else keeps the textual path. Capability gating lives in the message builder, so read_file never needs the model caps and degrades cleanly on a text-only model. - MessageBuilder inlines current-turn tool media as a synthetic user message right after the tool-result group (media_turn_start boundary, so older turns are never re-billed), reusing media.rs primitives via a new inline_paths helper that contains against the caller's workspace roots. OpenAI forwards the parts verbatim; the Anthropic client now also translates the PDF `file` part into a native `document` block (image_url → image was already handled). - read_file's description is annotated per serving model in call_llm_round, listing the formats it can open, so the model knows reading one shows it the content. Tests: media sniff (PDF), PDF file-part build, inline_paths containment + capability gating, capability hint, read_file media-vs-text, Anthropic file→document, and the owner-schema-stands-alone check with the new column. --- crates/core-api/src/tool.rs | 50 +++- crates/llm-client/src/anthropic.rs | 37 +++ crates/skald-core/src/db/chat_llm_tools.rs | 37 ++- crates/skald-core/src/db/mod.rs | 4 +- .../src/session/handler/llm_call.rs | 28 +- .../skald-core/src/session/handler/media.rs | 256 +++++++++++++++++- .../src/session/handler/message_builder.rs | 35 +++ .../src/session/handler/messages.rs | 3 + .../skald-core/src/session/handler/outcome.rs | 8 + crates/skald-core/src/tools/fs/mod.rs | 48 +++- crates/skald-core/src/tools/fs/read_file.rs | 58 +++- crates/skald-core/src/tools/mod.rs | 2 +- 12 files changed, 524 insertions(+), 42 deletions(-) diff --git a/crates/core-api/src/tool.rs b/crates/core-api/src/tool.rs index 08d05d5..822f54a 100644 --- a/crates/core-api/src/tool.rs +++ b/crates/core-api/src/tool.rs @@ -238,28 +238,62 @@ pub enum ToolResult { Text(String), /// Structured JSON result (e.g. MCP `structuredContent`). Json(serde_json::Value), + /// A text note plus one or more media files the model may view natively + /// (image / video / PDF). At the LLM wire the `tool` message carries only + /// `text` (see [`to_wire`](Self::to_wire)); the media travels **out of band** + /// (persisted in `chat_llm_tools.media`) and is inlined by the message + /// builder as a following synthetic `user` message — but only for the + /// current turn and only when the resolved model declares the modality; + /// otherwise it is silently dropped and the note stands alone. + Media { text: String, media: Vec }, } impl ToolResult { /// Tag persisted in `chat_llm_tools.result_type` and sent over the WS as - /// `ServerEvent::ToolDone.result_type`. Either `"string"` or `"json"`. + /// `ServerEvent::ToolDone.result_type`. `Media` reports `"string"`: its wire + /// form *is* a plain text note, and the media is signalled out of band by the + /// `chat_llm_tools.media` column — so the frontend needs no new result type. pub fn kind(&self) -> &'static str { match self { - Self::Text(_) => "string", - Self::Json(_) => "json", + Self::Text(_) => "string", + Self::Json(_) => "json", + Self::Media { .. } => "string", } } /// Wire content for the LLM tool message: text as-is, Json serialized to a - /// compact JSON string. Both OpenAI and Anthropic encode tool results as - /// text/JSON, so this is the canonical string form persisted in - /// `chat_llm_tools.result` and replayed by the message builder. + /// compact JSON string, `Media` its text note. Both OpenAI and Anthropic + /// encode tool results as text/JSON, so this is the canonical string form + /// persisted in `chat_llm_tools.result` and replayed by the message builder. pub fn to_wire(&self) -> String { match self { - Self::Text(s) => s.clone(), - Self::Json(v) => serde_json::to_string(v).unwrap_or_else(|_| "null".to_string()), + Self::Text(s) => s.clone(), + Self::Json(v) => serde_json::to_string(v).unwrap_or_else(|_| "null".to_string()), + Self::Media { text, .. } => text.clone(), } } + + /// The media files this result carries (empty for `Text`/`Json`). The message + /// builder reads these to inline the files as native model input. + pub fn media(&self) -> &[MediaRef] { + match self { + Self::Media { media, .. } => media, + _ => &[], + } + } +} + +/// A reference to one media file a tool produced (e.g. `read_file` on an image). +/// Carries the **already-containment-checked** absolute host path so the message +/// builder can re-read + inline it, plus the sniffed MIME for display. Serialized +/// as JSON into the `chat_llm_tools.media` column. +#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] +pub struct MediaRef { + /// Absolute host path, resolved and containment-checked by the producing tool. + pub host_path: String, + /// Sniffed MIME type (`image/png`, `application/pdf`, …). Informational — the + /// media pipeline re-sniffs from the bytes before inlining, never trusting this. + pub mime: String, } impl From for ToolResult { diff --git a/crates/llm-client/src/anthropic.rs b/crates/llm-client/src/anthropic.rs index 34a981c..5da699a 100644 --- a/crates/llm-client/src/anthropic.rs +++ b/crates/llm-client/src/anthropic.rs @@ -181,6 +181,11 @@ fn convert_user_content(content: &Value) -> Value { blocks.push(block); } } + "file" => { + if let Some(block) = parse_data_document(&p["file"]) { + blocks.push(block); + } + } other => tracing::warn!(part_type = other, "dropping content part unsupported by Anthropic"), } } @@ -198,6 +203,18 @@ fn parse_data_image(image_url: &Value) -> Option { })) } +/// `{"file_data": "data:application/pdf;base64,"}` → an Anthropic base64 +/// `document` block (the native PDF input). Only base64 data URLs are supported; +/// the OpenAI `file` part is what the media pipeline emits for a PDF. +fn parse_data_document(file: &Value) -> Option { + let url = file["file_data"].as_str()?; + let (mime, data) = url.strip_prefix("data:")?.split_once(";base64,")?; + Some(json!({ + "type": "document", + "source": { "type": "base64", "media_type": mime, "data": data }, + })) +} + #[async_trait] impl ChatbotClient for AnthropicClient { async fn chat( @@ -435,4 +452,24 @@ mod tests { ])); assert_eq!(v, json!([{ "type": "text", "text": "t" }])); } + + #[test] + fn user_content_file_part_becomes_document_block() { + // The OpenAI `file` part (emitted by the media pipeline for a PDF) becomes + // an Anthropic native `document` block. + let v = convert_user_content(&json!([ + { "type": "text", "text": "read this" }, + { "type": "file", "file": { "filename": "a.pdf", "file_data": "data:application/pdf;base64,QUJD" } }, + ])); + assert_eq!(v, json!([ + { "type": "text", "text": "read this" }, + { "type": "document", "source": { "type": "base64", "media_type": "application/pdf", "data": "QUJD" } }, + ])); + + // A non-data file_data (or missing) is dropped, not forwarded. + let v = convert_user_content(&json!([ + { "type": "file", "file": { "filename": "a.pdf", "file_data": "https://example.com/a.pdf" } }, + ])); + assert_eq!(v, json!([])); + } } diff --git a/crates/skald-core/src/db/chat_llm_tools.rs b/crates/skald-core/src/db/chat_llm_tools.rs index ee2b7ea..3d48b91 100644 --- a/crates/skald-core/src/db/chat_llm_tools.rs +++ b/crates/skald-core/src/db/chat_llm_tools.rs @@ -18,6 +18,10 @@ pub struct LlmToolCall { /// size cap (no diff shown then). Only populated by `for_message` (history). pub preview_old: Option, pub preview_new: Option, + /// JSON `[{host_path, mime}]` — media files this tool produced (e.g. `read_file` + /// on an image), to be inlined to the model as native input by the message + /// builder. `None` for non-media tools. Only populated by `for_message`/`get`. + pub media: Option, } /// Inserts a tool call in `running` state and returns its id. @@ -81,6 +85,19 @@ pub async fn set_preview( Ok(()) } +/// Persists the JSON media manifest for a tool that produced viewable media +/// (`ToolResult::Media`). Separate from [`complete`] — like [`set_preview`] — so the +/// out-of-band media write stays independent of the status/result write. Read back +/// by `for_message` so the message builder can inline the files for the model. +pub async fn set_media(pool: &SqlitePool, id: i64, media_json: &str) -> anyhow::Result<()> { + sqlx::query("UPDATE chat_llm_tools SET media = ? WHERE id = ?") + .bind(media_json) + .bind(id) + .execute(pool) + .await?; + Ok(()) +} + pub async fn fail(pool: &SqlitePool, id: i64, error: &str) -> anyhow::Result<()> { sqlx::query( "UPDATE chat_llm_tools SET result = ?, status = 'failed' WHERE id = ?", @@ -150,8 +167,8 @@ pub async fn for_message( pool: &SqlitePool, message_id: i64, ) -> anyhow::Result> { - let rows = sqlx::query_as::<_, (i64, i64, String, Option, Option, String, String, Option, Option)>( - "SELECT id, message_id, name, arguments, result, result_type, status, preview_old, preview_new + let rows = sqlx::query_as::<_, (i64, i64, String, Option, Option, String, String, Option, Option, Option)>( + "SELECT id, message_id, name, arguments, result, result_type, status, preview_old, preview_new, media FROM chat_llm_tools WHERE message_id = ? ORDER BY id ASC", @@ -161,8 +178,8 @@ pub async fn for_message( .await?; Ok(rows.into_iter() - .map(|(id, message_id, name, arguments, result, result_type, status, preview_old, preview_new)| { - LlmToolCall { id, message_id, name, arguments, result, result_type, status, preview_old, preview_new } + .map(|(id, message_id, name, arguments, result, result_type, status, preview_old, preview_new, media)| { + LlmToolCall { id, message_id, name, arguments, result, result_type, status, preview_old, preview_new, media } }) .collect()) } @@ -170,8 +187,8 @@ pub async fn for_message( /// A single tool call by id, with its diff-preview columns. Backs the tool-detail /// page (`GET /api/tools/{id}`). Returns `None` when the id is unknown in this pool. pub async fn get(pool: &SqlitePool, id: i64) -> anyhow::Result> { - let row = sqlx::query_as::<_, (i64, i64, String, Option, Option, String, String, Option, Option)>( - "SELECT id, message_id, name, arguments, result, result_type, status, preview_old, preview_new + let row = sqlx::query_as::<_, (i64, i64, String, Option, Option, String, String, Option, Option, Option)>( + "SELECT id, message_id, name, arguments, result, result_type, status, preview_old, preview_new, media FROM chat_llm_tools WHERE id = ?", ) @@ -179,8 +196,8 @@ pub async fn get(pool: &SqlitePool, id: i64) -> anyhow::Result, Option, String, String, ), ) -> LlmToolCall { - // The resume path (`pending_for_stack`) never needs the diff preview. - LlmToolCall { id, message_id, name, arguments, result, result_type, status, preview_old: None, preview_new: None } + // The resume path (`pending_for_stack`) never needs the diff preview or media. + LlmToolCall { id, message_id, name, arguments, result, result_type, status, preview_old: None, preview_new: None, media: None } } diff --git a/crates/skald-core/src/db/mod.rs b/crates/skald-core/src/db/mod.rs index 2a6ca37..fd39be7 100644 --- a/crates/skald-core/src/db/mod.rs +++ b/crates/skald-core/src/db/mod.rs @@ -750,14 +750,16 @@ pub async fn create_owner_tables(pool: &SqlitePool) -> Result<()> { result_type TEXT NOT NULL DEFAULT 'string' CHECK(result_type IN ('string', 'json')), preview_old TEXT, -- file-write diff: content before the write preview_new TEXT, -- file-write diff: content after the write + media TEXT, -- JSON [{host_path,mime}]: media the tool produced, inlined to the model out of band created_at TEXT NOT NULL DEFAULT (datetime('now')) )", ) .execute(pool) .await?; - // Diff-preview columns are additive — reach an already-created table in place. + // Diff-preview + tool-media columns are additive — reach an already-created table in place. ensure_column(pool, "chat_llm_tools", "preview_old", "TEXT").await?; ensure_column(pool, "chat_llm_tools", "preview_new", "TEXT").await?; + ensure_column(pool, "chat_llm_tools", "media", "TEXT").await?; sqlx::query( "CREATE INDEX IF NOT EXISTS idx_stack_session ON chat_sessions_stack(session_id)", diff --git a/crates/skald-core/src/session/handler/llm_call.rs b/crates/skald-core/src/session/handler/llm_call.rs index cc23963..9de0483 100644 --- a/crates/skald-core/src/session/handler/llm_call.rs +++ b/crates/skald-core/src/session/handler/llm_call.rs @@ -66,13 +66,20 @@ impl ChatSessionHandler { request_id: Some(request_id.clone()), }; + // Tell the model, in read_file's description, which media formats it can + // open directly — keyed on the model actually serving this attempt, so a + // fallback to a text-only model drops the claim. `None` (no media + // capability) leaves the shared defs untouched, avoiding a clone. + let annotated = media_annotated_tools(tool_defs, &cur_llm.capabilities); + let defs: &[Value] = annotated.as_deref().unwrap_or(tool_defs); + // Clone the Arc so the in-flight future does not borrow `cur_llm` across // the fallback reassignment below. On cancel we drop the future // (aborting the request) and return immediately. let client = cur_llm.client.clone(); let call_result = tokio::select! { _ = token.cancelled() => return RoundLlm::Cancelled, - r = client.chat_with_tools_raw(messages.as_slice(), tool_defs, &options) => r, + r = client.chat_with_tools_raw(messages.as_slice(), defs, &options) => r, }; let e = match call_result { @@ -163,6 +170,25 @@ fn first_line(s: &str) -> String { s.lines().next().unwrap_or(s).to_string() } +/// Appends a per-model media hint to `read_file`'s description when the resolved +/// model can view images/video/PDFs, so the model knows reading one of those shows +/// it the content natively. Returns `None` (leaving the shared, model-independent +/// defs untouched — no clone) when the model has no media modality. Done here, per +/// attempt, so a fallback to a different model re-derives the hint from its caps. +fn media_annotated_tools(tool_defs: &[Value], capabilities: &[String]) -> Option> { + let hint = super::media::media_capability_hint(capabilities)?; + let mut out = tool_defs.to_vec(); + for def in &mut out { + if def["function"]["name"].as_str() == Some("read_file") { + if let Some(d) = def["function"]["description"].as_str() { + def["function"]["description"] = Value::String(format!("{d}{hint}")); + } + break; + } + } + Some(out) +} + #[cfg(test)] mod tests { use super::is_retriable_llm_error; diff --git a/crates/skald-core/src/session/handler/media.rs b/crates/skald-core/src/session/handler/media.rs index 0dec385..7894614 100644 --- a/crates/skald-core/src/session/handler/media.rs +++ b/crates/skald-core/src/session/handler/media.rs @@ -18,13 +18,15 @@ //! //! Anything failing a check silently stays on the textual path. -use std::path::Path; +use std::path::{Path, PathBuf}; use base64::Engine as _; use serde_json::{json, Value}; use tracing::debug; use core_api::message_meta::Attachment; +use core_api::tool::MediaRef; +use core_api::user_fs::UserFs; /// Max media parts inlined per turn. const MAX_MEDIA_PER_TURN: usize = 4; @@ -32,16 +34,20 @@ const MAX_MEDIA_PER_TURN: usize = 4; const MAX_IMAGE_BYTES: u64 = 10 * 1024 * 1024; /// Max bytes for one inlined video. const MAX_VIDEO_BYTES: u64 = 32 * 1024 * 1024; +/// Max bytes for one inlined PDF (Anthropic's per-request document ceiling). +const MAX_PDF_BYTES: u64 = 32 * 1024 * 1024; /// Max combined media bytes inlined per turn. const MAX_TOTAL_MEDIA_BYTES: u64 = 48 * 1024 * 1024; /// A model-input modality: the capability that unlocks it, the content-part -/// type it maps to, its byte cap and the sniffed MIME types accepted. +/// type it maps to, its byte cap, the sniffed MIME types accepted, and a +/// human-readable format list for the `read_file` description. struct Modality { capability: &'static str, part_type: &'static str, max_bytes: u64, mimes: &'static [&'static str], + formats: &'static str, } const MODALITIES: &[Modality] = &[ @@ -50,6 +56,7 @@ const MODALITIES: &[Modality] = &[ part_type: "image_url", max_bytes: MAX_IMAGE_BYTES, mimes: &["image/png", "image/jpeg", "image/gif", "image/webp"], + formats: "images (PNG, JPEG, GIF, WebP)", }, Modality { capability: "video", @@ -64,9 +71,34 @@ const MODALITIES: &[Modality] = &[ "video/x-flv", "video/3gpp", ], + formats: "video (MP4, WebM, MOV, …)", + }, + // PDF documents. The `file` part is the OpenAI file-input shape + // (`{"type":"file","file":{"filename","file_data"}}`), forwarded verbatim by + // OpenAI-compatible clients and translated to a native `document` block by the + // Anthropic client. Gated on the `document` capability, so a model row without + // it (any OpenAI-compat endpoint that can't take a `file` part) never receives + // one — set the capability only on rows whose endpoint accepts PDFs. + Modality { + capability: "document", + part_type: "file", + max_bytes: MAX_PDF_BYTES, + mimes: &["application/pdf"], + formats: "PDF documents", }, ]; +/// Builds the OpenAI-wire content part for one inlined medium. Images/video use the +/// `{"type":"image_url"|"video_url","…":{"url":data-URL}}` shape; PDFs use the +/// `file` shape carrying a filename + `file_data` data-URL. +fn build_media_part(part_type: &str, mime: &str, b64: &str, filename: &str) -> Value { + let url = format!("data:{mime};base64,{b64}"); + match part_type { + "file" => json!({ "type": "file", "file": { "filename": filename, "file_data": url } }), + t => json!({ "type": t, t: { "url": url } }), + } +} + /// The result of partitioning a message's attachments. pub struct MediaPartition { /// OpenAI-style content parts, ready to append after the text part. @@ -117,8 +149,9 @@ pub async fn partition_under( MediaPartition { parts, rest } } -/// Promotes one attachment to a content part, or `None` when any check fails -/// (logged at debug level; the caller keeps it on the textual path). +/// Promotes one uploaded attachment to a content part, or `None` when any check +/// fails (logged at debug level; the caller keeps it on the textual path). +/// Containment is against the uploads `root`; the rest is [`promote`]. async fn try_inline( a: &Attachment, capabilities: &[String], @@ -131,32 +164,146 @@ async fn try_inline( debug!(path = %a.path, "media not inlined: outside the uploads root"); return None; } + promote(&abs, &a.name, capabilities, used_total).await +} - let mut file = tokio::fs::File::open(&abs).await.ok()?; +/// Read + sniff + capability/budget check + build the content part for one file at +/// an **already-contained** absolute path. Shared by the uploaded-attachment path +/// ([`try_inline`]) and the tool-produced-media path ([`inline_paths`]); neither +/// containment nor per-turn count budget is enforced here — the callers do that. +/// `None` (logged at debug) when the file is not a recognized medium, the model +/// lacks the modality, or a byte budget is exhausted. +async fn promote( + abs: &Path, + filename: &str, + capabilities: &[String], + used_total: u64, +) -> Option<(Value, u64)> { + let mut file = tokio::fs::File::open(abs).await.ok()?; let mut head = [0u8; 16]; let n = tokio::io::AsyncReadExt::read(&mut file, &mut head).await.ok()?; let mime = sniff_mime(&head[..n])?; let modality = MODALITIES.iter().find(|m| m.mimes.contains(&mime))?; if !capabilities.iter().any(|c| c == modality.capability) { - debug!(path = %a.path, mime, "media not inlined: model lacks the capability"); + debug!(path = %abs.display(), mime, "media not inlined: model lacks the capability"); return None; } let size = file.metadata().await.ok()?.len(); if size > modality.max_bytes { - debug!(path = %a.path, size, "media not inlined: file too large"); + debug!(path = %abs.display(), size, "media not inlined: file too large"); return None; } if used_total + size > MAX_TOTAL_MEDIA_BYTES { - debug!(path = %a.path, "media not inlined: per-turn byte budget exhausted"); + debug!(path = %abs.display(), "media not inlined: per-turn byte budget exhausted"); return None; } - let bytes = tokio::fs::read(&abs).await.ok()?; + let bytes = tokio::fs::read(abs).await.ok()?; let b64 = base64::engine::general_purpose::STANDARD.encode(&bytes); - let url = format!("data:{mime};base64,{b64}"); - let t = modality.part_type; - Some((json!({ "type": t, t: { "url": url } }), size)) + Some((build_media_part(modality.part_type, mime, &b64, filename), size)) +} + +/// Inline media a tool produced (e.g. `read_file` on an image) as content parts, +/// for the current turn only. Mirrors [`partition_under`] but contains against the +/// caller's **workspace roots** (home + shared + projects + docs) rather than the +/// uploads dir — the tool already resolved + contained the path, so this is a +/// fail-closed re-check against a symlink swap since the read (§6). Same per-file, +/// per-count and per-turn byte budgets; the capability gate lives here, so a +/// tool always records the media and the model only sees it when able. +pub async fn inline_paths( + refs: &[MediaRef], + capabilities: &[String], + fs: &UserFs, +) -> Vec { + let capable = MODALITIES + .iter() + .any(|m| capabilities.iter().any(|c| c == m.capability)); + if !capable || refs.is_empty() { + return Vec::new(); + } + let roots = workspace_roots(fs); + if roots.is_empty() { + return Vec::new(); + } + + let mut parts: Vec = Vec::new(); + let mut total: u64 = 0; + for r in refs { + if parts.len() >= MAX_MEDIA_PER_TURN { + break; + } + let canon = crate::tools::fs::canonicalize_for_policy(&r.host_path, Path::new("/")); + if !roots.iter().any(|root| crate::tools::fs::path_under(&canon, root)) { + debug!(path = %r.host_path, "tool media not inlined: outside the workspace"); + continue; + } + let filename = canon + .file_name() + .map(|s| s.to_string_lossy().into_owned()) + .unwrap_or_else(|| "file".to_string()); + if let Some((part, bytes)) = promote(&canon, &filename, capabilities, total).await { + total += bytes; + parts.push(part); + } + } + parts +} + +/// The caller's workspace roots, canonicalized for prefix-checking: private home, +/// each shared folder, each project, and the read-only docs mount. +fn workspace_roots(fs: &UserFs) -> Vec { + let canon = |p: &Path| crate::tools::fs::canonicalize_for_policy(&p.to_string_lossy(), Path::new("/")); + let mut roots = vec![canon(&fs.home_host)]; + for m in &fs.shared { + roots.push(canon(&m.host)); + } + for m in &fs.projects { + roots.push(canon(&m.host)); + } + if let Some(d) = &fs.docs_host { + roots.push(canon(d)); + } + roots +} + +/// Sentence appended to `read_file`'s description when the resolved model can view +/// media, naming the formats it takes as native input. `None` when the model has +/// no media modality (description stays unchanged). See `call_llm_round`. +pub fn media_capability_hint(capabilities: &[String]) -> Option { + let forms: Vec<&'static str> = MODALITIES + .iter() + .filter(|m| capabilities.iter().any(|c| c == m.capability)) + .map(|m| m.formats) + .collect(); + if forms.is_empty() { + return None; + } + Some(format!( + " This model can view {} directly: when you read_file one of these, its content is given to you as native model input (not text).", + join_human(&forms), + )) +} + +/// `["a"] → "a"`, `["a","b"] → "a and b"`, `["a","b","c"] → "a, b, and c"`. +fn join_human(items: &[&str]) -> String { + match items { + [] => String::new(), + [a] => a.to_string(), + [a, b] => format!("{a} and {b}"), + [rest @ .., last] => format!("{}, and {last}", rest.join(", ")), + } +} + +/// Opens a file and sniffs its first bytes, returning a recognized media MIME +/// (`image/*`, `video/*`, `application/pdf`) or `None` for an ordinary/unreadable +/// file. Used by `read_file` to decide whether to hand a file back as native media +/// rather than trying to read it as UTF-8 text. +pub async fn probe_media(path: &Path) -> Option<&'static str> { + let mut file = tokio::fs::File::open(path).await.ok()?; + let mut head = [0u8; 16]; + let n = tokio::io::AsyncReadExt::read(&mut file, &mut head).await.ok()?; + sniff_mime(&head[..n]) } /// Sniffs the magic bytes of a medium we know how to inline, returning its @@ -199,6 +346,9 @@ pub fn sniff_mime(head: &[u8]) -> Option<&'static str> { if head.starts_with(&[0x00, 0x00, 0x01, 0xBA]) || head.starts_with(&[0x00, 0x00, 0x01, 0xB3]) { return Some("video/mpeg"); } + if head.starts_with(b"%PDF-") { + return Some("application/pdf"); + } None } @@ -238,7 +388,7 @@ mod tests { assert_eq!(sniff_mime(b"RIFF\x00\x00\x00\x00AVI "), Some("video/x-msvideo")); assert_eq!(sniff_mime(b"FLV\x01\x05"), Some("video/x-flv")); assert_eq!(sniff_mime(&[0x00, 0x00, 0x01, 0xBA]), Some("video/mpeg")); - assert_eq!(sniff_mime(b"%PDF-1.7"), None); + assert_eq!(sniff_mime(b"%PDF-1.7"), Some("application/pdf")); assert_eq!(sniff_mime(b""), None); } @@ -305,4 +455,84 @@ mod tests { let _ = tokio::fs::remove_dir_all(&tmp).await; } + + fn pdf_bytes() -> Vec { + let mut v = b"%PDF-1.7\n".to_vec(); + v.extend_from_slice(&[0x00; 64]); + v + } + + #[tokio::test] + async fn partition_inlines_pdf_as_file_part_for_document_model() { + let tmp = std::env::temp_dir().join(format!("skald-media-{}", uuid::Uuid::new_v4())); + let dir = tmp.join("data/uploads/u/1"); + tokio::fs::create_dir_all(&dir).await.unwrap(); + tokio::fs::write(dir.join("a.pdf"), pdf_bytes()).await.unwrap(); + + // A document-capable model inlines the PDF as the OpenAI `file` part shape. + let p = partition_under(&[att("data/uploads/u/1/a.pdf")], &caps(&["document"]), &tmp).await; + assert!(p.rest.is_empty()); + assert_eq!(p.parts.len(), 1); + assert_eq!(p.parts[0]["type"], "file"); + assert_eq!(p.parts[0]["file"]["filename"], "a.pdf"); + let fd = p.parts[0]["file"]["file_data"].as_str().unwrap(); + assert!(fd.starts_with("data:application/pdf;base64,"), "{fd}"); + + // vision alone does not unlock PDFs. + let p = partition_under(&[att("data/uploads/u/1/a.pdf")], &caps(&["vision"]), &tmp).await; + assert_eq!(p.rest.len(), 1); + assert!(p.parts.is_empty()); + + let _ = tokio::fs::remove_dir_all(&tmp).await; + } + + /// A throwaway [`UserFs`] whose private home is `root/homes/u1`. + fn fs_home(home: &std::path::Path) -> UserFs { + UserFs::new( + "u1", + home.to_path_buf(), + "skald-u1", + PathBuf::from("/root"), + vec![], + vec![], + None, + ) + } + + #[tokio::test] + async fn inline_paths_contains_and_gates_on_capability() { + let tmp = std::env::temp_dir().join(format!("skald-toolmedia-{}", uuid::Uuid::new_v4())); + let home = tmp.join("homes/u1"); + tokio::fs::create_dir_all(&home).await.unwrap(); + tokio::fs::write(home.join("pic.png"), png_bytes()).await.unwrap(); + tokio::fs::write(tmp.join("outside.png"), png_bytes()).await.unwrap(); + let fs = fs_home(&home); + + let inside = MediaRef { host_path: home.join("pic.png").to_string_lossy().into_owned(), mime: "image/png".into() }; + let outside = MediaRef { host_path: tmp.join("outside.png").to_string_lossy().into_owned(), mime: "image/png".into() }; + + // capable + inside the home → one image part. + let parts = inline_paths(std::slice::from_ref(&inside), &caps(&["vision"]), &fs).await; + assert_eq!(parts.len(), 1); + assert_eq!(parts[0]["type"], "image_url"); + assert!(parts[0]["image_url"]["url"].as_str().unwrap().starts_with("data:image/png;base64,")); + + // no capability → nothing inlined. + assert!(inline_paths(std::slice::from_ref(&inside), &caps(&[]), &fs).await.is_empty()); + + // a real image outside the workspace is rejected fail-closed. + assert!(inline_paths(std::slice::from_ref(&outside), &caps(&["vision"]), &fs).await.is_empty()); + + let _ = tokio::fs::remove_dir_all(&tmp).await; + } + + #[test] + fn media_capability_hint_lists_enabled_formats_only() { + assert!(media_capability_hint(&caps(&[])).is_none()); + let h = media_capability_hint(&caps(&["vision"])).unwrap(); + assert!(h.contains("images (PNG, JPEG, GIF, WebP)"), "{h}"); + assert!(!h.contains("PDF"), "{h}"); + let h = media_capability_hint(&caps(&["vision", "document"])).unwrap(); + assert!(h.contains("images (PNG, JPEG, GIF, WebP)") && h.contains("PDF documents"), "{h}"); + } } diff --git a/crates/skald-core/src/session/handler/message_builder.rs b/crates/skald-core/src/session/handler/message_builder.rs index 9a05fb1..0667c49 100644 --- a/crates/skald-core/src/session/handler/message_builder.rs +++ b/crates/skald-core/src/session/handler/message_builder.rs @@ -4,6 +4,9 @@ use std::sync::Arc; use serde_json::{Value, json}; use sqlx::SqlitePool; +use core_api::tool::MediaRef; +use core_api::user_fs::UserFs; + use crate::compactor::{ContextCompactor, SUMMARY_PREFIX}; use crate::config::DatetimeConfig; use crate::db::{chat_history, chat_llm_tools, chat_summaries}; @@ -52,6 +55,11 @@ pub struct MessageBuilder { /// paths. `None` for non-project sessions, in which case an `inject_memory` /// entry that references `__PROJECT_ROOT__` is skipped (with a warning). pub project_root: Option, + /// The caller's filesystem view — its workspace roots contain (fail-closed) + /// the media a tool produced (`read_file` on an image/PDF) before it is inlined + /// for the model. `None` in the inert/ownerless bundle and unit tests that + /// don't exercise tool media (media inlining is then skipped). + pub fs: Option>, } impl MessageBuilder { @@ -352,6 +360,33 @@ impl MessageBuilder { "content": result_content, })); } + + // Media a tool produced this turn (e.g. read_file on an + // image/PDF): inline it as a synthetic `user` message right + // after the tool-result group, so a capable model sees the + // bytes. Reuses the user-attachment translation path in each + // client (OpenAI verbatim; Anthropic image/document blocks). + // Current turn only (`idx >= media_turn_start`) — older-turn + // media stays the textual note, never re-billed. `inline_paths` + // gates on the model's capability + budgets + containment. + if idx >= media_turn_start + && let Some(fs) = self.fs.as_deref() + { + let mut refs: Vec = Vec::new(); + for tc in &tool_calls { + if let Some(mj) = &tc.media + && let Ok(mut v) = serde_json::from_str::>(mj) + { + refs.append(&mut v); + } + } + if !refs.is_empty() { + let parts = super::media::inline_paths(&refs, capabilities, fs).await; + if !parts.is_empty() { + out.push(json!({ "role": "user", "content": parts })); + } + } + } } } } diff --git a/crates/skald-core/src/session/handler/messages.rs b/crates/skald-core/src/session/handler/messages.rs index ee3a01d..69611da 100644 --- a/crates/skald-core/src/session/handler/messages.rs +++ b/crates/skald-core/src/session/handler/messages.rs @@ -38,6 +38,9 @@ impl ChatSessionHandler { max_tool_result_chars: self.max_tool_result_chars, compactor: self.compactor.clone(), project_root, + // Snapshot the fs cell for this build — its workspace roots contain the + // tool-produced media inlined into the current turn (§6 remount-safe). + fs: Some(self.fs.load()), }; // `pool` is passed in from the caller (always `&self.db`) but we take // ownership via Arc::clone above so the signature stays backward-compatible. diff --git a/crates/skald-core/src/session/handler/outcome.rs b/crates/skald-core/src/session/handler/outcome.rs index c820afb..d2882e2 100644 --- a/crates/skald-core/src/session/handler/outcome.rs +++ b/crates/skald-core/src/session/handler/outcome.rs @@ -50,6 +50,14 @@ impl ChatSessionHandler { let kind = result.kind(); debug!(session_id = self.session_id, tool = %tool_name, tool_call_id, result_len = wire.len(), "tool done"); chat_llm_tools::complete(pool, tool_call_id, &wire, kind).await?; + // Media the tool produced (e.g. read_file on an image/PDF) rides + // out of band in the `media` column; the message builder inlines it + // as a synthetic user message for a capable model on the current turn. + let media = result.media(); + if !media.is_empty() { + let media_json = serde_json::to_string(media).unwrap_or_else(|_| "[]".to_string()); + chat_llm_tools::set_media(pool, tool_call_id, &media_json).await?; + } // Persist a file-write's diff snapshot so it re-renders after a reload, // and carry it on the event so an auto-allowed write shows the diff live. let (preview_old, preview_new) = match preview { diff --git a/crates/skald-core/src/tools/fs/mod.rs b/crates/skald-core/src/tools/fs/mod.rs index f6e5647..2c1a778 100644 --- a/crates/skald-core/src/tools/fs/mod.rs +++ b/crates/skald-core/src/tools/fs/mod.rs @@ -277,7 +277,7 @@ mod tests { use core_api::user_fs::UserFs; - use crate::tools::{ExecutionOutcome, Tool, ToolContext}; + use crate::tools::{ExecutionOutcome, Tool, ToolContext, ToolResult}; /// A trivial workspace for the memory-routing tests, which never touch disk. fn test_fs() -> Arc { @@ -551,4 +551,50 @@ mod tests { let _ = std::fs::remove_dir_all(&udir); let _ = std::fs::remove_dir_all(&sdir); } + + /// A physical `read_file` on a binary image hands the file back as + /// `ToolResult::Media` (host path + sniffed MIME) instead of failing on the + /// non-UTF-8 bytes; a UTF-8 file still reads as line-numbered text. + #[tokio::test] + async fn read_file_returns_media_for_binary_image() { + let (shared, sdir) = store("readmedia-shared").await; + let (user, udir) = store("readmedia-user").await; + + let root = std::env::temp_dir().join(format!("skald-readmedia-{}", uuid::Uuid::new_v4())); + let home = root.join("homes").join("u1"); + std::fs::create_dir_all(&home).unwrap(); + let mut png = b"\x89PNG\r\n\x1a\n".to_vec(); + png.extend_from_slice(&[0xAA; 64]); + std::fs::write(home.join("pic.png"), &png).unwrap(); + std::fs::write(home.join("note.txt"), "hello\nworld").unwrap(); + + let fs = Arc::new(UserFs::new( + "u1", home.clone(), "skald-u1", PathBuf::from("/root"), vec![], vec![], None, + )); + let ctx = ToolContext { session_id: 1, user_id: "u1".into(), pool: Arc::clone(&user), fs }; + let read = ReadFile::new(Arc::clone(&shared)); + + // image → Media, carrying the resolved host path + MIME. + match read.run_with(&ctx, json!({"path": "~/pic.png"})).wait().await { + ExecutionOutcome::Completed(ToolResult::Media { text, media }) => { + assert!(text.contains("binary media") && text.contains("image/png"), "{text}"); + assert_eq!(media.len(), 1); + assert_eq!(media[0].mime, "image/png"); + assert!(media[0].host_path.ends_with("pic.png"), "{}", media[0].host_path); + } + other => panic!("expected Media, got {other:?}"), + } + + // UTF-8 text → ordinary numbered text. + match read.run_with(&ctx, json!({"path": "~/note.txt"})).wait().await { + ExecutionOutcome::Completed(ToolResult::Text(t)) => { + assert!(t.contains("| hello") && t.contains("| world"), "{t}"); + } + other => panic!("expected Text, got {other:?}"), + } + + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&udir); + let _ = std::fs::remove_dir_all(&sdir); + } } diff --git a/crates/skald-core/src/tools/fs/read_file.rs b/crates/skald-core/src/tools/fs/read_file.rs index 6f57303..83b0d92 100644 --- a/crates/skald-core/src/tools/fs/read_file.rs +++ b/crates/skald-core/src/tools/fs/read_file.rs @@ -1,11 +1,11 @@ use std::sync::Arc; -use anyhow::Result; +use anyhow::{Context, Result}; use serde_json::{Value, json}; use sqlx::SqlitePool; use crate::tools::{ - SimpleExecution, Tool, ToolContext, ToolDescriptionLength, ToolExecution, ToolResult, + MediaRef, SimpleExecution, Tool, ToolContext, ToolDescriptionLength, ToolExecution, ToolResult, truncate_label, MAX_LABEL_SHORT, MAX_LABEL_FULL, }; use super::{classify_memory, read_to_string, MemScope}; @@ -47,6 +47,26 @@ fn number_lines(content: &str, start: usize, end_line: Option, limit: Opt .join("\n") } +/// A short, honest note returned as the `tool` message when `read_file` opens a +/// binary medium. The bytes travel out of band (`ToolResult::Media`); this text is +/// what the model reads in the tool result itself. +fn media_note(agent_path: &str, mime: &str, size: u64) -> String { + format!( + "[read_file: {agent_path} is binary media ({mime}, {}). It is provided to you directly as model input when the current model supports this format; it cannot be shown as text.]", + human_size(size), + ) +} + +/// `1536 → "1.5 KiB"`, `2_100_000 → "2.0 MiB"`. +fn human_size(bytes: u64) -> String { + const KIB: f64 = 1024.0; + const MIB: f64 = 1024.0 * 1024.0; + let b = bytes as f64; + if b >= MIB { format!("{:.1} MiB", b / MIB) } + else if b >= KIB { format!("{:.1} KiB", b / KIB) } + else { format!("{bytes} B") } +} + impl Tool for ReadFile { fn name(&self) -> &str { "read_file" } fn display_name(&self) -> &str { "Read File" } @@ -110,15 +130,39 @@ impl Tool for ReadFile { } } - /// Routes `user-memory/…` / `shared-memory/…` to the note store; every other - /// path falls through to the on-disk [`execute`](Self::execute). + /// Routes `user-memory/…` / `shared-memory/…` to the note store; a physical + /// path resolves to the caller's host workspace and is read there — as native + /// media when it sniffs as an image/video/PDF (so a vision/document model can + /// see it), otherwise as UTF-8 text with line numbers. fn run_with<'a>(&'a self, ctx: &ToolContext, args: Value) -> Box { let path = super::path_arg(&args).unwrap_or_default(); let Some(m) = classify_memory(&path) else { - return match super::rewrite_to_host(&ctx.fs, &path, args) { - Ok(args) => self.run(args), - Err(e) => super::error_exec(e.to_string()), + // Physical path: resolve + containment-check up front (so an escape + // fails immediately), then read inside the work future. + let host = match super::resolve_host_path(&ctx.fs, &path) { + Ok(h) => h, + Err(e) => return super::error_exec(e.to_string()), }; + let start = args["start_line"].as_u64().map(|n| (n as usize).saturating_sub(1)).unwrap_or(0); + let end_line = args["end_line"].as_u64().map(|n| n as usize); + let limit = args["limit"].as_u64().map(|n| n.min(2000) as usize); + return Box::new(SimpleExecution::new(Box::pin(async move { + // A recognized medium is handed back for native inlining rather than + // failing on non-UTF-8 bytes. We always emit the media (the message + // builder gates on the resolved model's capability), so on a model + // without the modality the note stands alone — never a decode error. + if let Some(mime) = crate::session::handler::media::probe_media(&host).await { + let size = tokio::fs::metadata(&host).await.map(|m| m.len()).unwrap_or(0); + let host_str = host.to_string_lossy().into_owned(); + return Ok(ToolResult::Media { + text: media_note(&path, mime, size), + media: vec![MediaRef { host_path: host_str, mime: mime.to_string() }], + }); + } + let content = tokio::fs::read_to_string(&host).await + .with_context(|| format!("Cannot read file: {path}"))?; + Ok(ToolResult::Text(number_lines(&content, start, end_line, limit))) + }))); }; let pool = match m.scope { MemScope::User => Arc::clone(&ctx.pool), diff --git a/crates/skald-core/src/tools/mod.rs b/crates/skald-core/src/tools/mod.rs index cdc74bd..d74321c 100644 --- a/crates/skald-core/src/tools/mod.rs +++ b/crates/skald-core/src/tools/mod.rs @@ -53,7 +53,7 @@ use anyhow::Result; use serde_json::Value; pub use core_api::tool::{ - drive_execution, ExecutionOutcome, SimpleExecution, Tool, ToolCategory, ToolContext, + drive_execution, ExecutionOutcome, MediaRef, SimpleExecution, Tool, ToolCategory, ToolContext, ToolDescriptionLength, ToolExecution, ToolResult, truncate_label, };