fix: one tool-set recipe per session, so a tool cannot vanish between rounds
Nightly Build / build (push) Successful in 7m36s

Two "unknown tool (not in this turn's tool set)" failures, one disease: the
turn's tool set was rebuilt from a different recipe depending on which entry
point happened to drive it.

A sub-agent got `ask_user_clarification`, `execute_subtask` and `activate_tools`
and nothing else — while `agents/common/tools.md` and every reporting agent's
prompt tell it to register its output with `update_scratchpad`. The child could
see the scratchpad injected into its context but had no way to write to it. It
now gets the scratchpad and todos tools, on the parent's `scratchpad_sid`: one
blackboard per session, as the surrounding code already declared.

`show_file_to_user` was injected per message by the WS handler, while
`resume_session` and `resolve_pending_call` rebuilt the list with `execute_task`
alone. So approving a card, or reconnecting mid-turn, continued the *same*
conversation with the tool silently gone. There is now a single recipe,
`ChatHub::session_interface_tools`, used by all three paths and fed by a builder
the shell installs once through `Skald::set_interface_tools_builder`: the core
keeps owning the tool, the shell keeps owning the policy of who gets it —
Telegram still does not, since it cannot act on OpenFile.
This commit is contained in:
2026-08-04 15:03:46 +01:00
parent ff298f1aef
commit f900d803f2
8 changed files with 163 additions and 36 deletions
+5
View File
@@ -61,6 +61,11 @@ mod tools;
/// check and the registration id can never drift apart. /// check and the registration id can never drift apart.
pub(crate) const PLUGIN_ID: &str = "telegram"; 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. /// Injected as extra system context for every Telegram turn.
/// Kept compact to minimise token overhead. /// Kept compact to minimise token overhead.
pub(crate) const TELEGRAM_FORMAT_CONTEXT: &str = "\ pub(crate) const TELEGRAM_FORMAT_CONTEXT: &str = "\
+57 -21
View File
@@ -44,6 +44,21 @@ const NOTIFY_BATCH_WINDOW_MS: u64 = 200;
// first message of a burst. // first message of a burst.
const SOURCE_COALESCE_DEBOUNCE_MS: u64 = 0; 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<ChatHub>, &str, &Arc<ChatSessionHandler>) -> Vec<InterfaceTool> + Send + Sync,
>;
// ── ChatHub ─────────────────────────────────────────────────────────────────── // ── ChatHub ───────────────────────────────────────────────────────────────────
/// Manages **interactive, user-facing sessions only** (web, mobile, project chats): /// 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. /// TaskManager reference for injecting execute_task into interactive sessions.
/// Set via set_task_mgr() after construction (breaks circular dep with cron). /// Set via set_task_mgr() after construction (breaks circular dep with cron).
task_mgr: std::sync::OnceLock<Arc<TaskManager>>, task_mgr: std::sync::OnceLock<Arc<TaskManager>>,
/// The surface's own interface tools, installed post-construction by the
/// shell. See [`InterfaceToolsBuilder`].
iface_tools: OnceLock<InterfaceToolsBuilder>,
/// Per-source input inboxes (coalescing + FIFO ordering). Created lazily on the /// Per-source input inboxes (coalescing + FIFO ordering). Created lazily on the
/// first message for a source; each spawns one consumer task. /// first message for a source; each spawns one consumer task.
inboxes: Mutex<HashMap<String, Arc<SourceInbox>>>, inboxes: Mutex<HashMap<String, Arc<SourceInbox>>>,
@@ -109,6 +127,7 @@ impl ChatHub {
global_tx, global_tx,
notify_tx, notify_tx,
task_mgr: std::sync::OnceLock::new(), task_mgr: std::sync::OnceLock::new(),
iface_tools: OnceLock::new(),
inboxes: Mutex::new(HashMap::new()), inboxes: Mutex::new(HashMap::new()),
me: OnceLock::new(), me: OnceLock::new(),
shutdown: shutdown.clone(), shutdown: shutdown.clone(),
@@ -131,6 +150,12 @@ impl ChatHub {
let _ = self.task_mgr.set(task_mgr); 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 ──────────────────────────────────────────────────────────── // ── Public API ────────────────────────────────────────────────────────────
/// Register a source. No-op for duplicate registrations. /// 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. // 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); 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 // get_or_create_handler is idempotent; we call it early because the
// session's RunContext so it can be inherited by any task spawned here. // 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 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. // The session's own interface tools — the same recipe the resume and
// session_id and run_context_json are captured so tasks inherit the parent context. // 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; let mut interface_tools = opts.interface_tools;
if let Some(task_mgr) = self.task_mgr.get() { interface_tools.extend(
interface_tools.push( self.session_interface_tools(session_id, source_id, &handler).await,
crate::tools::cron_jobs::build_execute_task_interface_tool(
Arc::clone(task_mgr),
session_id,
run_context_json,
)
); );
}
handler.handle_message( handler.handle_message(
prompt, prompt,
opts.client_name, opts.client_name,
@@ -407,9 +428,9 @@ impl ChatHub {
let source = chat_sessions::find_by_id(&self.db, session_id).await? let source = chat_sessions::find_by_id(&self.db, session_id).await?
.map(|s| s.source) .map(|s| s.source)
.unwrap_or_else(|| "web".to_string()); .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 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 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? let source = chat_sessions::find_by_id(&self.db, session_id).await?
.map(|s| s.source) .map(|s| s.source)
.unwrap_or_else(|| "web".to_string()); .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 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 handler.resolve_pending_call(call, decision, interface_tools, tx).await
} }
/// Builds the `execute_task` interface tool for a session, mirroring the injection /// **The** interface-tool recipe of a session: `execute_task` (so a pending
/// done for live turns. Empty when no TaskManager is configured /// sub-agent task can be re-dispatched) plus whatever the surface declared
/// so `execute_task mode=async` can be rebuilt by `build_execution` during resume. /// through [`Self::set_interface_tools_builder`].
async fn execute_task_tools( ///
/// 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, &self,
session_id: i64, session_id: i64,
source: &str,
handler: &Arc<ChatSessionHandler>, handler: &Arc<ChatSessionHandler>,
) -> Vec<InterfaceTool> { ) -> Vec<InterfaceTool> {
let mut tools = Vec::new(); let mut tools = Vec::new();
@@ -455,6 +486,11 @@ impl ChatHub {
run_context_json, 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 tools
} }
+15 -1
View File
@@ -26,7 +26,9 @@ use crate::clarification::ClarificationManager;
use crate::llm::LlmManager; use crate::llm::LlmManager;
use crate::llm::logging::RequestLogTarget; use crate::llm::logging::RequestLogTarget;
use crate::loop_adapters::activation::SkaldToolActivator; 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::history::SqliteHistory;
use crate::loop_adapters::prefix_cache::PrefixCache; use crate::loop_adapters::prefix_cache::PrefixCache;
use crate::loop_adapters::runtime::LoopConfig; use crate::loop_adapters::runtime::LoopConfig;
@@ -217,6 +219,18 @@ impl AgentCatalog for SkaldAgentCatalog {
scope.session_id, scope.session_id,
Some(child_frame.get()), 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<dyn agent_loop::tool::ToolSet> = Arc::new( let toolset: Arc<dyn agent_loop::tool::ToolSet> = Arc::new(
SkaldToolSet::new( SkaldToolSet::new(
@@ -366,6 +366,22 @@ impl ChatSessionHandler {
self.fs.load() 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<SqlitePool> {
&self.db
}
/// The registry pool (`system.db`) — shared memory, registry tables.
pub fn shared_pool(&self) -> &Arc<SqlitePool> {
&self.shared_pool
}
/// Override the session used for scratchpad reads/writes. /// Override the session used for scratchpad reads/writes.
/// Called by the cron runner for async tasks so they share the parent's scratchpad. /// Called by the cron runner for async tasks so they share the parent's scratchpad.
pub fn set_scratchpad_session_id(&self, id: i64) { pub fn set_scratchpad_session_id(&self, id: i64) {
+10
View File
@@ -63,6 +63,16 @@ impl Skald {
fn rt_user_contexts(&self) -> &super::user_context::UserContextRegistry { &self.user_contexts } 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** /// 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 /// 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. /// logged in has no snapshot to refresh; their next login builds a fresh one.
+25 -1
View File
@@ -21,7 +21,7 @@
//! pending map. //! pending map.
use std::collections::HashMap; use std::collections::HashMap;
use std::sync::Arc; use std::sync::{Arc, OnceLock};
use anyhow::Result; use anyhow::Result;
use chrono_tz::Tz; use chrono_tz::Tz;
@@ -146,6 +146,10 @@ pub(super) struct UserContextFactory {
datetime_config: DatetimeConfig, datetime_config: DatetimeConfig,
compaction: CompactionConfig, compaction: CompactionConfig,
cron_tz: Option<Tz>, cron_tz: Option<Tz>,
/// 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<crate::chat_hub::InterfaceToolsBuilder>,
} }
impl UserContextFactory { impl UserContextFactory {
@@ -181,9 +185,19 @@ impl UserContextFactory {
datetime_config: DatetimeConfig { timezone: config.timezone.clone(), ..config.llm.datetime }, datetime_config: DatetimeConfig { timezone: config.timezone.clone(), ..config.llm.datetime },
compaction: config.llm.compaction.clone(), compaction: config.llm.compaction.clone(),
cron_tz, 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<Arc<UserContext>> { async fn build(&self, user_id: &str, pool: SqlitePool) -> Result<Arc<UserContext>> {
let pool = Arc::new(pool); let pool = Arc::new(pool);
// This user's own stop signal: a **child** of the instance token, so a global // This user's own stop signal: a **child** of the instance token, so a global
@@ -352,6 +366,9 @@ impl UserContextFactory {
user_shutdown.clone(), user_shutdown.clone(),
default_agent, 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("web").await;
chat_hub.register("talk").await; chat_hub.register("talk").await;
@@ -405,6 +422,13 @@ impl UserContextRegistry {
Self { factory, contexts: Mutex::new(HashMap::new()) } 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: /// Returns the user's context, building it from `pool` on first use. Idempotent:
/// once built, the same `Arc<UserContext>` is returned until restart. /// once built, the same `Arc<UserContext>` is returned until restart.
pub(super) async fn resolve(&self, user_id: &str, pool: SqlitePool) -> Result<Arc<UserContext>> { pub(super) async fn resolve(&self, user_id: &str, pool: SqlitePool) -> Result<Arc<UserContext>> {
+5 -12
View File
@@ -404,18 +404,11 @@ async fn handle_socket(mut socket: WebSocket, skald: Arc<Skald>, source: String,
// set_selected_client, which broadcasts ClientSelected. // set_selected_client, which broadcasts ClientSelected.
client_name: chat_hub.get_selected_client(&source).await, client_name: chat_hub.get_selected_client(&source).await,
extra_system_context: Some(WEB_FORMAT_CONTEXT.to_string()), extra_system_context: Some(WEB_FORMAT_CONTEXT.to_string()),
// SPA-only tool: lets the assistant open a file in the user's // `show_file_to_user` used to be injected right here, per
// viewer. Injected here (not in the registry) so it exists only // message — which is why it disappeared from a conversation
// for ws.rs clients (web + mobile), never for the Telegram plugin. // the moment an approval or a reconnect resumed the turn
interface_tools: vec![ // through another path. It is now declared once, for every
skald_core::tools::show_file::make_tool( // path, by `WebFrontend::interface_tools_builder`.
Arc::clone(&chat_hub),
source.clone(),
ctx.fs.clone(),
ctx.pool.as_ref().clone(),
skald.db().as_ref().clone(),
),
],
..Default::default() ..Default::default()
}; };
// send_message only enqueues — the turn runs on ChatHub's per-source // send_message only enqueues — the turn runs on ChatHub's per-source
+29
View File
@@ -10,6 +10,7 @@ use tracing::{error, info};
use core_api::plugin::RouterFactory; use core_api::plugin::RouterFactory;
use crate::frontend::config::FrontendConfig; use crate::frontend::config::FrontendConfig;
use skald_core::chat_hub::InterfaceToolsBuilder;
use skald_core::skald::Skald; use skald_core::skald::Skald;
use crate::frontend::server::{WebServer, WebServerHandle}; 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<WebServerHandle> { pub async fn start(self) -> Result<WebServerHandle> {
// 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(). // 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_router_factory(self.make_router_factory());
self.skald.plugin_manager().set_web_port(self.port); self.skald.plugin_manager().set_web_port(self.port);