From 55dcb482994361bdd4e80088184ef5878e9f9bf6 Mon Sep 17 00:00:00 2001 From: Daniele Date: Mon, 10 Aug 2026 00:08:16 +0100 Subject: [PATCH] fix(telegram): resolve send_attachment paths in the user's workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `send_attachment` handed its `file_path` argument straight to `InputFile::file`, which resolves against the **server process's** working directory. Every path the model can actually have — relative to the user's home, or absolute inside their container — failed the `path.exists()` check, and the one class that didn't (a name that happens to exist next to the binary) would have sent the wrong file. The routing already exists for the fs-tools, so expose it rather than repeat it: `UserFilesApi` (core-api) reads a path in the agent's own vocabulary and is obtained from `UserChannelHandle::files()`, so it is scoped to one user by construction. skald-core implements it over `resolve_view_target` — host mount read directly, container-only path through `docker exec` — holding the `SharedFs` cell rather than a snapshot, so a remount lands without a login. The size cap is checked before the read (a new `exec_fs::size` for the container branch): the point of a cap is to keep an oversized file out of RAM, so checking it afterwards would protect nothing. A photo above `sendPhoto`'s narrower 10 MB ceiling goes out as a document instead of as an API error. --- crates/core-api/src/lib.rs | 1 + crates/core-api/src/user_channel.rs | 8 +++ crates/core-api/src/user_files.rs | 42 ++++++++++++++ crates/plugin-telegram-bot/src/handlers.rs | 4 +- crates/plugin-telegram-bot/src/tools.rs | 50 ++++++++++++---- crates/skald-core/src/container/exec_fs.rs | 14 +++++ crates/skald-core/src/skald/user_context.rs | 64 +++++++++++++++++++++ 7 files changed, 171 insertions(+), 12 deletions(-) create mode 100644 crates/core-api/src/user_files.rs diff --git a/crates/core-api/src/lib.rs b/crates/core-api/src/lib.rs index bdd5bd7..82be9b9 100644 --- a/crates/core-api/src/lib.rs +++ b/crates/core-api/src/lib.rs @@ -22,6 +22,7 @@ pub mod provider; pub mod remote; pub mod tool; pub mod user_channel; +pub mod user_files; pub mod user_fs; pub mod user_plugin_config; pub mod secrets; diff --git a/crates/core-api/src/user_channel.rs b/crates/core-api/src/user_channel.rs index b89cf0b..556a962 100644 --- a/crates/core-api/src/user_channel.rs +++ b/crates/core-api/src/user_channel.rs @@ -23,6 +23,7 @@ use crate::approval::ApprovalApi; use crate::chat_hub::ChatHubApi; use crate::events::GlobalEvent; use crate::inbox::InboxApi; +use crate::user_files::UserFilesApi; /// Resolves an unlocked user's channel handle. /// @@ -84,6 +85,13 @@ pub trait UserChannelHandle: Send + Sync { /// `approval()`/clarification/elicitation separately. fn inbox(&self) -> Arc; + /// The user's workspace files — reading a path in the agent's own vocabulary + /// (`~/…`, `shared/{X}/…`, `/tmp/…`), routed to the host mount or to the + /// container exactly as the fs-tools route it. A channel adapter that sends a + /// file back to the user goes through this rather than the host filesystem, + /// whose cwd is the server's and not the user's. + fn files(&self) -> Arc; + /// Subscribe to the user's server→client event stream. /// Events are scoped to this user; no cross-user leakage. fn subscribe(&self) -> broadcast::Receiver; diff --git a/crates/core-api/src/user_files.rs b/crates/core-api/src/user_files.rs new file mode 100644 index 0000000..61237b4 --- /dev/null +++ b/crates/core-api/src/user_files.rs @@ -0,0 +1,42 @@ +//! Reading a user's files from a channel plugin (blueprint §6). +//! +//! A channel adapter that hands a file back to the user — Telegram's +//! `send_attachment` is the first — is given a path in the **agent's** vocabulary +//! (`~/report.pdf`, `uploads/{session}/photo.jpg`, `shared/{X}/…`, or a +//! container-absolute `/tmp/out.png`), because that is the only vocabulary the +//! model has ever seen. None of those spellings is a host path: resolving them +//! means the same two-backing routing the fs-tools do — a bind-mounted path read +//! host-side, anything else read through the user's container. +//! +//! That routing lives in the core, so this is the seam that lets a plugin borrow +//! it instead of touching the process working directory (which is what a plain +//! `std::fs::read` of an agent path does — it either fails or, worse, reads a +//! same-named file next to the binary). + +use async_trait::async_trait; + +/// A file read out of a user's workspace. +pub struct UserFile { + /// The canonical agent-vocabulary path — what the user and the model see. + pub display: String, + /// Basename of [`display`](Self::display), for surfaces that need a file name. + pub name: String, + pub bytes: Vec, +} + +/// Reads files from one user's workspace, with the agent's own path routing. +/// +/// Obtained from [`UserChannelHandle::files`](crate::user_channel::UserChannelHandle::files), +/// so it is already scoped to that user: containment is the core's +/// (canonicalize + prefix-check on the mounts, the container otherwise) and a +/// path outside the caller's view is refused, never silently resolved elsewhere. +#[async_trait] +pub trait UserFilesApi: Send + Sync { + /// Reads `path`, refusing anything larger than `max_bytes` **before** loading + /// it — the cap is the caller's own limit (Telegram's upload ceiling, say), + /// and a size check that ran after the read would protect nothing. + /// + /// Virtual memory notes (`user-memory/…`, `shared-memory/…`) are not files and + /// are rejected with a clear error. + async fn read(&self, path: &str, max_bytes: u64) -> anyhow::Result; +} diff --git a/crates/plugin-telegram-bot/src/handlers.rs b/crates/plugin-telegram-bot/src/handlers.rs index c6a509f..70477f5 100644 --- a/crates/plugin-telegram-bot/src/handlers.rs +++ b/crates/plugin-telegram-bot/src/handlers.rs @@ -418,7 +418,9 @@ async fn handle_llm_message( client_name, extra_system_context: Some(TELEGRAM_FORMAT_CONTEXT.to_string()), tail_reminder: Some(super::TELEGRAM_FORMAT_REMINDER.to_string()), - interface_tools: super::tools::interface_tools(bot.clone(), chat_id, &*shared.tts).await, + interface_tools: super::tools::interface_tools( + bot.clone(), chat_id, &*shared.tts, handle.files(), + ).await, metadata, ..Default::default() }; diff --git a/crates/plugin-telegram-bot/src/tools.rs b/crates/plugin-telegram-bot/src/tools.rs index 3f42919..ee8110d 100644 --- a/crates/plugin-telegram-bot/src/tools.rs +++ b/crates/plugin-telegram-bot/src/tools.rs @@ -8,6 +8,7 @@ use teloxide::types::InputFile; use core_api::interface_tool::InterfaceTool; use core_api::tool::{Tool, ToolCategory, ToolDescriptionLength}; use core_api::tts::{TextToSpeech, TtsProvider}; +use core_api::user_files::UserFilesApi; use super::auth::{Binding, load_config, save_config}; use super::TelegramPlugin; @@ -26,8 +27,9 @@ pub(crate) async fn interface_tools( bot: Bot, chat_id: ChatId, tts: &dyn TtsProvider, + files: Arc, ) -> Vec { - let mut tools = vec![send_attachment_tool(bot.clone(), chat_id)]; + let mut tools = vec![send_attachment_tool(bot.clone(), chat_id, files)]; if let Some(synth) = tts.get().await { tools.push(send_voice_tool(bot, chat_id, synth)); @@ -38,19 +40,37 @@ pub(crate) async fn interface_tools( // ── send_attachment ─────────────────────────────────────────────────────────── -fn send_attachment_tool(bot: Bot, chat_id: ChatId) -> InterfaceTool { +/// What the Bot API accepts in one upload (50 MB). Checked before the file is +/// read, so an oversized one costs a `stat` rather than a rejected 50 MB POST. +const TELEGRAM_UPLOAD_LIMIT: u64 = 50 * 1000 * 1000; + +/// The narrower ceiling `sendPhoto` enforces — above it an image is sent as a +/// document instead, which is the same bytes without the inline preview. +const TELEGRAM_PHOTO_LIMIT: u64 = 10 * 1000 * 1000; + +/// Sends a file from the **user's** workspace, resolved through +/// [`UserFilesApi`] — the same routing the fs-tools use, so `~/report.pdf`, +/// `uploads/{session}/photo.jpg` and the container-only `/tmp/out.png` all work. +/// +/// It used to hand the raw argument to `InputFile::file`, which resolves against +/// the **server process's** working directory: every agent path the model has +/// ever been given (each of them relative to the user's home, or absolute inside +/// their container) failed the `path.exists()` check, and the one class that did +/// not — a name that happens to exist next to the binary — would have sent the +/// wrong file entirely. +fn send_attachment_tool(bot: Bot, chat_id: ChatId, files: Arc) -> InterfaceTool { InterfaceTool { definition: json!({ "type": "function", "function": { "name": "send_attachment", - "description": "Send a file from the local filesystem to the user on Telegram. Images (jpg/png/webp) and videos (mp4/mov/webm) are sent inline by default; any other type is sent as a document. Set as_document=true to force sending as a downloadable file.", + "description": "Send a file to the user on Telegram. Images (jpg/png/webp) and videos (mp4/mov/webm) are sent inline by default; any other type is sent as a document. Set as_document=true to force sending as a downloadable file.", "parameters": { "type": "object", "properties": { "file_path": { "type": "string", - "description": "Absolute or relative path to the file to send." + "description": "Path to the file, in your usual vocabulary: `~/report.pdf`, `uploads/…`, `shared/{folder}/…`, `projects/…`, or an absolute path inside your sandbox (`/tmp/out.png`). Memory notes cannot be sent." }, "caption": { "type": "string", @@ -67,6 +87,7 @@ fn send_attachment_tool(bot: Bot, chat_id: ChatId) -> InterfaceTool { }), handler: Arc::new(move |args| { let bot = bot.clone(); + let files = Arc::clone(&files); Box::pin(async move { let file_path = args["file_path"] .as_str() @@ -74,18 +95,17 @@ fn send_attachment_tool(bot: Bot, chat_id: ChatId) -> InterfaceTool { let caption = args["caption"].as_str().map(str::to_string); let as_document = args["as_document"].as_bool().unwrap_or(false); - let path = std::path::Path::new(file_path); - if !path.exists() { - anyhow::bail!("send_attachment: file not found: {file_path}"); - } + let read = files.read(file_path, TELEGRAM_UPLOAD_LIMIT).await + .map_err(|e| anyhow::anyhow!("send_attachment: {e}"))?; // Present images/videos inline by default; everything else (and // anything when as_document=true) as a downloadable document. - let ext = path.extension() + let ext = std::path::Path::new(&read.name) + .extension() .and_then(|e| e.to_str()) .unwrap_or("") .to_ascii_lowercase(); - let kind = if as_document { + let mut kind = if as_document { "document" } else { match ext.as_str() { @@ -94,8 +114,16 @@ fn send_attachment_tool(bot: Bot, chat_id: ChatId) -> InterfaceTool { _ => "document", } }; + // `sendPhoto` caps at 10 MB where `sendDocument` takes 50, so a big + // image goes out as a file rather than as an API error. + if kind == "photo" && read.bytes.len() as u64 > TELEGRAM_PHOTO_LIMIT { + kind = "document"; + } - let file = InputFile::file(path); + // The bytes are already in hand — a container file has no host path + // to point Telegram at, and a mounted one would only be re-read. + let file = InputFile::memory(read.bytes).file_name(read.name); + let file_path = read.display; let result = match kind { "photo" => { let mut req = bot.send_photo(chat_id, file); diff --git a/crates/skald-core/src/container/exec_fs.rs b/crates/skald-core/src/container/exec_fs.rs index 1a9df5e..dfdb9d4 100644 --- a/crates/skald-core/src/container/exec_fs.rs +++ b/crates/skald-core/src/container/exec_fs.rs @@ -93,6 +93,20 @@ pub async fn write(container: &str, path: &Path, bytes: &[u8]) -> Result<()> { Ok(()) } +/// Byte size of a file inside the container — for the callers that must decide +/// whether to read it *before* pulling it through the pipe. `wc -c` rather than +/// `stat`, so the answer is the same on any of the image's shells. +pub async fn size(container: &str, path: &Path) -> Result { + let p = path.to_string_lossy(); + let raw = sh(container, r#"wc -c < "$1""#, &[&p]) + .await + .with_context(|| format!("Cannot stat file: {p}"))?; + String::from_utf8_lossy(&raw) + .trim() + .parse() + .with_context(|| format!("Cannot stat file: {p}")) +} + pub async fn exists(container: &str, path: &Path) -> bool { sh_ok(container, r#"test -e "$1""#, &[&path.to_string_lossy()]).await } diff --git a/crates/skald-core/src/skald/user_context.rs b/crates/skald-core/src/skald/user_context.rs index ffae9aa..b7775ff 100644 --- a/crates/skald-core/src/skald/user_context.rs +++ b/crates/skald-core/src/skald/user_context.rs @@ -35,6 +35,7 @@ use core_api::events::GlobalEvent; use core_api::inbox::InboxApi; use core_api::system_bus::SystemEventBus; use core_api::user_channel::UserChannelHandle; +use core_api::user_files::{UserFile, UserFilesApi}; use core_api::user_fs::SharedFs; use crate::approval::ApprovalManager; @@ -560,7 +561,70 @@ impl UserChannelHandle for UserContextHandle { Arc::new(self.ctx.inbox.clone()) as Arc } + fn files(&self) -> Arc { + Arc::new(UserContextFiles { fs: self.ctx.fs.clone() }) as Arc + } + fn subscribe(&self) -> broadcast::Receiver { self.ctx.global_tx.subscribe() } } + +// ── UserFilesApi impl ───────────────────────────────────────────────────────── + +/// Reads one user's files for a channel plugin, with the fs-tools' own routing. +/// +/// It holds the [`SharedFs`] rather than a snapshot of it, so a membership change +/// that remounts the user's container (§6) is picked up on the next read instead +/// of at the next login. +struct UserContextFiles { + fs: SharedFs, +} + +#[async_trait::async_trait] +impl UserFilesApi for UserContextFiles { + async fn read(&self, path: &str, max_bytes: u64) -> Result { + let fs = self.fs.load(); + let (target, display) = crate::tools::fs::resolve_view_target(fs.as_ref(), path)?; + let name = std::path::Path::new(&display) + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_else(|| display.clone()); + + // Size first, in both branches: the cap exists to keep an oversized file + // out of RAM, so checking it after the read would be decoration. + let too_big = |size: u64| { + anyhow::anyhow!( + "{display} is {:.1} MB — larger than the {:.0} MB this can send", + size as f64 / 1e6, + max_bytes as f64 / 1e6, + ) + }; + + let bytes = match target { + crate::tools::fs::FsTarget::Host(abs) => { + let meta = tokio::fs::metadata(&abs) + .await + .map_err(|_| anyhow::anyhow!("file not found: {display}"))?; + if meta.is_dir() { + anyhow::bail!("{display} is a directory, not a file"); + } + if meta.len() > max_bytes { + anyhow::bail!(too_big(meta.len())); + } + tokio::fs::read(&abs).await? + } + crate::tools::fs::FsTarget::Container { container, path } => { + let size = crate::container::exec_fs::size(&container, &path) + .await + .map_err(|_| anyhow::anyhow!("file not found: {display}"))?; + if size > max_bytes { + anyhow::bail!(too_big(size)); + } + crate::container::exec_fs::read(&container, &path).await? + } + }; + + Ok(UserFile { display, name, bytes }) + } +}