diff --git a/crates/plugin-telegram-bot/src/lib.rs b/crates/plugin-telegram-bot/src/lib.rs index 134f7df..07053ab 100644 --- a/crates/plugin-telegram-bot/src/lib.rs +++ b/crates/plugin-telegram-bot/src/lib.rs @@ -61,6 +61,11 @@ mod tools; /// check and the registration id can never drift apart. pub(crate) const PLUGIN_ID: &str = "telegram"; +/// The chat source id this plugin owns. Exported so the shell can tell a +/// plugin-driven conversation from an SPA one when it declares which interface +/// tools a session gets (a Telegram client cannot act on `OpenFile`). +pub const SOURCE: &str = "telegram"; + /// Injected as extra system context for every Telegram turn. /// Kept compact to minimise token overhead. pub(crate) const TELEGRAM_FORMAT_CONTEXT: &str = "\ diff --git a/crates/skald-core/src/chat_hub/mod.rs b/crates/skald-core/src/chat_hub/mod.rs index 88aa543..c9c7fde 100644 --- a/crates/skald-core/src/chat_hub/mod.rs +++ b/crates/skald-core/src/chat_hub/mod.rs @@ -44,6 +44,21 @@ const NOTIFY_BATCH_WINDOW_MS: u64 = 200; // first message of a burst. const SOURCE_COALESCE_DEBOUNCE_MS: u64 = 0; +/// Builds the surface-specific interface tools of one session. +/// +/// The core owns the tools themselves but must never learn **which** surface +/// gets them (`show_file_to_user` is for SPA clients, never for the Telegram +/// plugin): that policy is installed by the shell through +/// [`ChatHub::set_interface_tools_builder`] and consulted by every path that +/// starts or resumes a turn, so the tool set of a conversation cannot depend on +/// which entry point drove it. +/// +/// The hub hands **itself** in as an argument rather than being captured, so a +/// builder stored on the hub is not a reference cycle. +pub type InterfaceToolsBuilder = Arc< + dyn Fn(Arc, &str, &Arc) -> Vec + Send + Sync, +>; + // ── ChatHub ─────────────────────────────────────────────────────────────────── /// Manages **interactive, user-facing sessions only** (web, mobile, project chats): @@ -68,6 +83,9 @@ pub struct ChatHub { /// TaskManager reference for injecting execute_task into interactive sessions. /// Set via set_task_mgr() after construction (breaks circular dep with cron). task_mgr: std::sync::OnceLock>, + /// The surface's own interface tools, installed post-construction by the + /// shell. See [`InterfaceToolsBuilder`]. + iface_tools: OnceLock, /// Per-source input inboxes (coalescing + FIFO ordering). Created lazily on the /// first message for a source; each spawns one consumer task. inboxes: Mutex>>, @@ -109,6 +127,7 @@ impl ChatHub { global_tx, notify_tx, task_mgr: std::sync::OnceLock::new(), + iface_tools: OnceLock::new(), inboxes: Mutex::new(HashMap::new()), me: OnceLock::new(), shutdown: shutdown.clone(), @@ -131,6 +150,12 @@ impl ChatHub { let _ = self.task_mgr.set(task_mgr); } + /// Installs the surface's interface-tool policy. Called once per hub by the + /// shell (the core must not know what an SPA is). Absent ⇒ no extra tools. + pub fn set_interface_tools_builder(&self, build: InterfaceToolsBuilder) { + let _ = self.iface_tools.set(build); + } + // ── Public API ──────────────────────────────────────────────────────────── /// Register a source. No-op for duplicate registrations. @@ -199,23 +224,19 @@ impl ChatHub { // Bridge mpsc from handle_message → global broadcast, tagging with source/session. let tx = Self::bridge_to_global(self.global_tx.clone(), source_tag, session_id); - // get_or_create_handler is idempotent; we call it early to read the - // session's RunContext so it can be inherited by any task spawned here. + // get_or_create_handler is idempotent; we call it early because the + // session's RunContext (read inside the recipe below) is inherited by + // any task spawned here. let handler = self.session_mgr.get_or_create_handler(session_id).await?; - let run_context_json = handler.run_context_json().await; - // Inject execute_task as an InterfaceTool for all interactive sessions. - // session_id and run_context_json are captured so tasks inherit the parent context. + // The session's own interface tools — the same recipe the resume and + // approval-resolution paths use, so nothing appears or vanishes + // depending on how the turn started. A caller may still add its own on + // top through `opts`. let mut interface_tools = opts.interface_tools; - if let Some(task_mgr) = self.task_mgr.get() { - interface_tools.push( - crate::tools::cron_jobs::build_execute_task_interface_tool( - Arc::clone(task_mgr), - session_id, - run_context_json, - ) - ); - } + interface_tools.extend( + self.session_interface_tools(session_id, source_id, &handler).await, + ); handler.handle_message( prompt, opts.client_name, @@ -407,9 +428,9 @@ impl ChatHub { let source = chat_sessions::find_by_id(&self.db, session_id).await? .map(|s| s.source) .unwrap_or_else(|| "web".to_string()); - let tx = Self::bridge_to_global(self.global_tx.clone(), source, session_id); + let tx = Self::bridge_to_global(self.global_tx.clone(), source.clone(), session_id); let handler = self.session_mgr.get_or_create_handler(session_id).await?; - let interface_tools = self.execute_task_tools(session_id, &handler).await; + let interface_tools = self.session_interface_tools(session_id, &source, &handler).await; handler.recover_turn(interface_tools, tx).await } @@ -432,18 +453,28 @@ impl ChatHub { let source = chat_sessions::find_by_id(&self.db, session_id).await? .map(|s| s.source) .unwrap_or_else(|| "web".to_string()); - let tx = Self::bridge_to_global(self.global_tx.clone(), source, session_id); + let tx = Self::bridge_to_global(self.global_tx.clone(), source.clone(), session_id); let handler = self.session_mgr.get_or_create_handler(session_id).await?; - let interface_tools = self.execute_task_tools(session_id, &handler).await; + let interface_tools = self.session_interface_tools(session_id, &source, &handler).await; handler.resolve_pending_call(call, decision, interface_tools, tx).await } - /// Builds the `execute_task` interface tool for a session, mirroring the injection - /// done for live turns. Empty when no TaskManager is configured - /// so `execute_task mode=async` can be rebuilt by `build_execution` during resume. - async fn execute_task_tools( + /// **The** interface-tool recipe of a session: `execute_task` (so a pending + /// sub-agent task can be re-dispatched) plus whatever the surface declared + /// through [`Self::set_interface_tools_builder`]. + /// + /// Every path that starts or resumes a turn goes through here — the live + /// message, `resume_session`, `resolve_pending_call`. That is the whole + /// point: before this, only the live path was given `show_file_to_user` + /// (injected per-message by the WS handler), so approving a card or + /// reconnecting mid-turn continued the *same conversation* with the tool + /// silently gone, and the model's next call to it failed with "unknown + /// tool". A tool set must be a property of the session, not of the entry + /// point that happened to drive it. + async fn session_interface_tools( &self, session_id: i64, + source: &str, handler: &Arc, ) -> Vec { let mut tools = Vec::new(); @@ -455,6 +486,11 @@ impl ChatHub { run_context_json, )); } + if let Some(build) = self.iface_tools.get() { + if let Some(me) = self.me.get().and_then(Weak::upgrade) { + tools.extend(build(me, source, handler)); + } + } tools } diff --git a/crates/skald-core/src/loop_adapters/catalog.rs b/crates/skald-core/src/loop_adapters/catalog.rs index d4b110e..d7cb450 100644 --- a/crates/skald-core/src/loop_adapters/catalog.rs +++ b/crates/skald-core/src/loop_adapters/catalog.rs @@ -26,7 +26,9 @@ use crate::clarification::ClarificationManager; use crate::llm::LlmManager; use crate::llm::logging::RequestLogTarget; use crate::loop_adapters::activation::SkaldToolActivator; -use crate::loop_adapters::builtins::{SkaldAskUserTool, SkaldHumanChannel}; +use crate::loop_adapters::builtins::{ + SkaldAskUserTool, SkaldHumanChannel, UpdateScratchpadTool, WriteTodosTool, +}; use crate::loop_adapters::history::SqliteHistory; use crate::loop_adapters::prefix_cache::PrefixCache; use crate::loop_adapters::runtime::LoopConfig; @@ -217,6 +219,18 @@ impl AgentCatalog for SkaldAgentCatalog { scope.session_id, Some(child_frame.get()), ))))); + // The blackboard and the checklist. Both are *told* to sub-agents by the + // prompts (`agents/common/tools.md`, and every reporting agent ends with + // "register your report with `update_scratchpad`"), and the scratchpad is + // injected into a child's context — so leaving the writers out of the + // child's tool set made a documented instruction unexecutable: the model + // called them and got "unknown tool". `scratchpad_sid` is the parent's, + // deliberately (see the context above): one blackboard per session. + native.push(Arc::new(UpdateScratchpadTool::new( + self.pool.clone(), + scope.scratchpad_sid, + ))); + native.push(Arc::new(WriteTodosTool)); let toolset: Arc = Arc::new( SkaldToolSet::new( diff --git a/crates/skald-core/src/session/handler/mod.rs b/crates/skald-core/src/session/handler/mod.rs index c373f4f..9571f66 100644 --- a/crates/skald-core/src/session/handler/mod.rs +++ b/crates/skald-core/src/session/handler/mod.rs @@ -366,6 +366,22 @@ impl ChatSessionHandler { self.fs.load() } + /// The swappable fs **cell**, not a snapshot: a tool built from it follows a + /// §6 remount instead of pinning the membership it saw at build time. + pub fn shared_fs(&self) -> SharedFs { + self.fs.clone() + } + + /// The owner's encrypted pool (`{userid}.db`). + pub fn owner_pool(&self) -> &Arc { + &self.db + } + + /// The registry pool (`system.db`) — shared memory, registry tables. + pub fn shared_pool(&self) -> &Arc { + &self.shared_pool + } + /// Override the session used for scratchpad reads/writes. /// Called by the cron runner for async tasks so they share the parent's scratchpad. pub fn set_scratchpad_session_id(&self, id: i64) { diff --git a/crates/skald-core/src/skald/accessors.rs b/crates/skald-core/src/skald/accessors.rs index 010a02d..06e67ae 100644 --- a/crates/skald-core/src/skald/accessors.rs +++ b/crates/skald-core/src/skald/accessors.rs @@ -63,6 +63,16 @@ impl Skald { fn rt_user_contexts(&self) -> &super::user_context::UserContextRegistry { &self.user_contexts } + /// Declares the tools the running surface contributes to a chat session + /// (the SPA's `show_file_to_user`, …). Called once by the shell after + /// construction: the core owns the tools, the shell owns the policy of who + /// gets them. Every per-user hub built from here on receives it, and every + /// path that starts or resumes a turn consults it — see + /// [`crate::chat_hub::InterfaceToolsBuilder`]. + pub fn set_interface_tools_builder(&self, build: crate::chat_hub::InterfaceToolsBuilder) { + self.rt_user_contexts().set_interface_tools_builder(build); + } + /// The user's runtime context IF it is already live (built), **without** /// building one — used to refresh a logged-in user in place. A user who never /// logged in has no snapshot to refresh; their next login builds a fresh one. diff --git a/crates/skald-core/src/skald/user_context.rs b/crates/skald-core/src/skald/user_context.rs index 5d7e7e7..c51ed9b 100644 --- a/crates/skald-core/src/skald/user_context.rs +++ b/crates/skald-core/src/skald/user_context.rs @@ -21,7 +21,7 @@ //! pending map. use std::collections::HashMap; -use std::sync::Arc; +use std::sync::{Arc, OnceLock}; use anyhow::Result; use chrono_tz::Tz; @@ -146,6 +146,10 @@ pub(super) struct UserContextFactory { datetime_config: DatetimeConfig, compaction: CompactionConfig, cron_tz: Option, + /// The surface's interface-tool policy, installed by the shell after + /// construction and handed to every per-user hub built from here — the one + /// place that can reach a hub created lazily at login. + iface_tools: OnceLock, } impl UserContextFactory { @@ -181,9 +185,19 @@ impl UserContextFactory { datetime_config: DatetimeConfig { timezone: config.timezone.clone(), ..config.llm.datetime }, compaction: config.llm.compaction.clone(), cron_tz, + iface_tools: OnceLock::new(), } } + /// Installs the shell's interface-tool policy. Applies to every hub built + /// from now on; call it before serving requests. + pub(super) fn set_interface_tools_builder( + &self, + build: crate::chat_hub::InterfaceToolsBuilder, + ) { + let _ = self.iface_tools.set(build); + } + async fn build(&self, user_id: &str, pool: SqlitePool) -> Result> { let pool = Arc::new(pool); // This user's own stop signal: a **child** of the instance token, so a global @@ -352,6 +366,9 @@ impl UserContextFactory { user_shutdown.clone(), default_agent, ); + if let Some(build) = self.iface_tools.get() { + chat_hub.set_interface_tools_builder(Arc::clone(build)); + } chat_hub.register("web").await; chat_hub.register("talk").await; @@ -405,6 +422,13 @@ impl UserContextRegistry { Self { factory, contexts: Mutex::new(HashMap::new()) } } + pub(super) fn set_interface_tools_builder( + &self, + build: crate::chat_hub::InterfaceToolsBuilder, + ) { + self.factory.set_interface_tools_builder(build); + } + /// Returns the user's context, building it from `pool` on first use. Idempotent: /// once built, the same `Arc` is returned until restart. pub(super) async fn resolve(&self, user_id: &str, pool: SqlitePool) -> Result> { diff --git a/src/frontend/api/ws.rs b/src/frontend/api/ws.rs index 8d9204d..edb6d32 100644 --- a/src/frontend/api/ws.rs +++ b/src/frontend/api/ws.rs @@ -404,18 +404,11 @@ async fn handle_socket(mut socket: WebSocket, skald: Arc, source: String, // set_selected_client, which broadcasts ClientSelected. client_name: chat_hub.get_selected_client(&source).await, extra_system_context: Some(WEB_FORMAT_CONTEXT.to_string()), - // SPA-only tool: lets the assistant open a file in the user's - // viewer. Injected here (not in the registry) so it exists only - // for ws.rs clients (web + mobile), never for the Telegram plugin. - interface_tools: vec![ - skald_core::tools::show_file::make_tool( - Arc::clone(&chat_hub), - source.clone(), - ctx.fs.clone(), - ctx.pool.as_ref().clone(), - skald.db().as_ref().clone(), - ), - ], + // `show_file_to_user` used to be injected right here, per + // message — which is why it disappeared from a conversation + // the moment an approval or a reconnect resumed the turn + // through another path. It is now declared once, for every + // path, by `WebFrontend::interface_tools_builder`. ..Default::default() }; // send_message only enqueues — the turn runs on ChatHub's per-source diff --git a/src/frontend/mod.rs b/src/frontend/mod.rs index c280961..4af077a 100644 --- a/src/frontend/mod.rs +++ b/src/frontend/mod.rs @@ -10,6 +10,7 @@ use tracing::{error, info}; use core_api::plugin::RouterFactory; use crate::frontend::config::FrontendConfig; +use skald_core::chat_hub::InterfaceToolsBuilder; use skald_core::skald::Skald; use crate::frontend::server::{WebServer, WebServerHandle}; @@ -43,7 +44,35 @@ impl WebFrontend { }) } + /// The tools this surface contributes to a chat session, whichever entry + /// point drives the turn (a live message, a reconnect, an approval answered + /// from the Inbox). + /// + /// `show_file_to_user` opens a file in the user's viewer by emitting + /// `ServerEvent::OpenFile` — only a WS client that renders the viewer can + /// act on it. The Telegram plugin ignores that event and has + /// `send_attachment` instead, so a Telegram conversation must not be + /// offered the tool: the model would report having shown a file nobody saw. + fn interface_tools_builder() -> InterfaceToolsBuilder { + Arc::new(|hub, source, handler| { + if source == plugin_telegram_bot::SOURCE { + return Vec::new(); + } + vec![skald_core::tools::show_file::make_tool( + hub, + source.to_string(), + handler.shared_fs(), + handler.owner_pool().as_ref().clone(), + handler.shared_pool().as_ref().clone(), + )] + }) + } + pub async fn start(self) -> Result { + // What the SPA lends to a session's tool set. Installed before anything + // can serve a request, so every per-user hub built at login gets it. + self.skald.set_interface_tools_builder(Self::interface_tools_builder()); + // Provide the router factory and web port to plugins before start_enabled(). self.skald.plugin_manager().set_router_factory(self.make_router_factory()); self.skald.plugin_manager().set_web_port(self.port);