fix: stop Telegram handing out pairing codes the store never saw

Pairing failed with "invalid or expired pairing code" on a code the bot
had just sent. handle_pairing read the pending codes from the in-memory
`shared.bindings` cache, which is refreshed from the ConfigKeyUpdated
broadcast — a lossy 64-slot bus. One dropped event is enough for that
cache to keep a pending entry the store no longer has; the "reuse an
existing code for this chat" branch then hits, and that branch does not
write. The user gets a code, and the web page — which resolves it
against the store — cannot find it. Before the move to the config store,
this path re-read the file on every message and could not drift.

The cache stays where it earns its keep, the chat_id → user_id lookup on
every inbound message, where a stale read costs one message. Issuing a
code now reads the store.

Two silent failures on the same path, each able to produce the same
symptom while hiding its cause:

handle_pairing sent the code even when the write had failed — it logged
and carried on — so the error surfaced later, somewhere else, as a code
that simply would not bind. It now says so in the chat and hands out
nothing.

load_config turned an unparseable blob into `unwrap_or_default()`: no
bindings, no pending codes. Every writer here saves the whole blob back,
so the next pairing message would have overwritten the real config with
that default and taken every binding on the box with it. An absent key
is still an empty config — that is a fresh install — but an unreadable
one is now an error that callers propagate, including start(), which
fails loudly rather than running on a cache it knows is wrong.

CLAUDE.md

@@ -130,7 +130,7 @@ Two rules keep the boundary real, and both are enforced by the compiler:
 The schema is split into two buckets (§5.1), and the split is the point:
 
 - **`create_registry_tables`** — instance-wide, readable without any user key: `users`, `roles`, `llm_providers`, `llm_models`, `transcribe_models`, `tts_models`, `image_generate_models`, `plugins`, `plugin_access` + `plugin_user_configs`, `approval_rules`, `tool_permission_groups`, `config`, `known_tools`, `llm_requests`, `mcp_catalog`, `mcp_global_servers` + `mcp_global_access`, `oauth_providers`, `role_capabilities`, `shared_folders` + `shared_folder_members`, `projects` + `project_members`, `supervision`, `system_agent_coverage`. The MCP tables back the Connectors model (§7/§14/§15 — see its own section); `oauth_providers` (accessor `db/oauth_providers.rs`) holds one row per identity provider (Google…) — endpoints + `client_id`/`client_secret` + `redirect_uri`, admin-owned household secrets (§4/§15b), never a per-user token. The last two pairs are junction-backed membership: `shared_folder_members` (accessor `db/shared_folders.rs`) for the on-disk shared folders (§6), `project_members` (accessor `db/project_members.rs`) for projects (see the Projects section) — both let a member be read-only (`can_write`) and both drive the container mount topology + the fs routing. Their FKs are registry→registry (same file), which is allowed — unlike an owner→registry key.
-- **`create_owner_tables`** — one owner's content, **identical schema in every file that has it**: `chat_sessions`, `chat_sessions_stack`, `chat_history`, `chat_llm_tools`, `chat_summaries`, `session_scratchpad`, `session_mcp_grants`, `stack_mcp_grants`, `scheduled_jobs`, `job_runs`, `system_agent_runs`, `system_agent_state`, `mcp_user_servers`, `mcp_events`, `sources`, `secrets`, `llm_request_payloads`, `memory_docs` (+ FTS5 `memory_docs_fts`), `reports`. `mcp_user_servers` (a user's activated per-user connectors) carries `catalog_name` as a **bare `TEXT` snapshot** of `mcp_catalog.name`, never a FK — an owner→registry key would fail every INSERT; for an OAuth connector it also snapshots `oauth_provider` + `deliver_json`, and its `api_key` column holds the refresh token (in the SQLCipher-encrypted file, so no column crypto). Because `memory_docs` is an owner table, one definition backs **private** memory in each `{userid}.db` and **shared** memory in `system.db` (the household owner) — see the memory namespace note below. (`projects`/`project_tickets` were owner tables in the single-user past: projects are shareable now, so `projects` + `project_members` are registry tables and `project_tickets` is gone.)
+- **`create_owner_tables`** — one owner's content, **identical schema in every file that has it**: `chat_sessions`, `chat_sessions_stack`, `chat_history`, `chat_llm_tools`, `chat_summaries`, `session_scratchpad`, `session_mcp_grants`, `stack_mcp_grants`, `scheduled_jobs`, `job_runs`, `system_agent_runs`, `system_agent_state`, `mcp_user_servers`, `mcp_events`, `sources`, `secrets`, `user_config`, `llm_request_payloads`, `memory_docs` (+ FTS5 `memory_docs_fts`), `reports`. `user_config` is the per-user twin of the registry `config` table and deliberately does **not** share its name: the two hold different namespaces (instance settings the admin owns vs. one member's own preferences, the notification home being the first), and a same-named table in both files would turn a wrong-pool call into a silent read of the other scope — instead of the "no such table: config" that revealed `/sethome` writing owner state through `db::config` against a `{userid}.db`, which also had the notification consumer dropping every batch it ever built. `mcp_user_servers` (a user's activated per-user connectors) carries `catalog_name` as a **bare `TEXT` snapshot** of `mcp_catalog.name`, never a FK — an owner→registry key would fail every INSERT; for an OAuth connector it also snapshots `oauth_provider` + `deliver_json`, and its `api_key` column holds the refresh token (in the SQLCipher-encrypted file, so no column crypto). Because `memory_docs` is an owner table, one definition backs **private** memory in each `{userid}.db` and **shared** memory in `system.db` (the household owner) — see the memory namespace note below. (`projects`/`project_tickets` were owner tables in the single-user past: projects are shareable now, so `projects` + `project_members` are registry tables and `project_tickets` is gone.)
 
 **The schema is no longer greenfield** (see the production note at the top): a full recreate is not an option anymore. `db::ensure_column` — `ALTER TABLE … ADD COLUMN` swallowing the "duplicate column" error, a no-op on a fresh DB where the `CREATE TABLE` already carries it — is therefore not a convenience for dev boxes anymore but the **only** change shape that is currently safe, and additive-with-a-default is the shape to design towards. Used for the OAuth columns on `mcp_catalog` / `mcp_user_servers`. Anything destructive waits for real versioning.
 

crates/plugin-telegram-bot/src/auth.rs

@@ -44,11 +44,22 @@ pub struct PairingEntry {
 
 // ── Config-table read/write ────────────────────────────────────────────────────
 
-/// Reads the Telegram config from the `config` table. Returns `Default` when
+/// Reads the Telegram config from the `config` table.
-/// the key is absent or unparseable (never fails the caller).
+///
+/// An **absent** key is an empty config — that is the state of a fresh install.
+/// An **unparseable** one is an error, deliberately: this used to be
+/// `unwrap_or_default()`, which turned a blob the current schema cannot read
+/// into "no bindings, no pending codes" — and since every writer here saves the
+/// whole blob back, the next pairing message would then overwrite the file with
+/// that default and every binding on the box would be gone for good. Failing
+/// loudly leaves the value intact for a human to look at.
 pub(crate) async fn load_config(config: &dyn ConfigApi) -> anyhow::Result<TelegramConfig> {
     match config.get(CONFIG_KEY).await? {
-        Some(json) => Ok(serde_json::from_str(&json).unwrap_or_default()),
+        Some(json) => serde_json::from_str(&json)
+            .map_err(|e| anyhow::anyhow!(
+                "telegram: the stored `{CONFIG_KEY}` config is not readable ({e}) — \
+                 refusing to overwrite it; inspect the `config` table"
+            )),
         None => Ok(TelegramConfig::default()),
     }
 }
@@ -70,8 +81,26 @@ const PAIRING_TTL_HOURS: i64 = 24;
 
 /// Called when an unbound `chat_id` sends a message. Generates (or reuses) a
 /// pairing code, persists it to the config table, and replies with instructions.
+///
+/// **Reads the store, not `shared.bindings`.** The cache is refreshed from a
+/// lossy 64-slot broadcast (`ConfigKeyUpdated`), so it may hold a pending code
+/// the store no longer has — a dropped event is enough. That cache is right for
+/// the hot `chat_id → user_id` lookup on every inbound message; it is wrong
+/// here, because the reader on the other side of the pairing (the web page and
+/// the `telegram_pairing` tool) resolves the code against the **store**, and a
+/// code handed out from a stale cache is one that can never bind: the user gets
+/// their code and the web answers "invalid or expired". Pairing happens once
+/// per person, so the extra read costs nothing.
 pub(crate) async fn handle_pairing(bot: &Bot, chat_id: ChatId, shared: &Arc<TgShared>) {
-    let mut cfg = shared.bindings.read().await.clone();
+    let mut cfg = match load_config(&*shared.config).await {
+        Ok(c) => c,
+        Err(e) => {
+            error!(error = %e, "telegram: cannot read the config to issue a pairing code");
+            bot.send_message(chat_id, "⚠️ Pairing is unavailable right now — please ask the admin to check the server.")
+                .await.ok();
+            return;
+        }
+    };
 
     // Prune expired codes.
     let cutoff = Utc::now() - chrono::Duration::hours(PAIRING_TTL_HOURS);
@@ -94,14 +123,19 @@ pub(crate) async fn handle_pairing(bot: &Bot, chat_id: ChatId, shared: &Arc<TgSh
     };
 
     if added {
+        // A code the store did not accept is worse than no code: the user pastes
+        // it, the web resolves it against the store, and the failure surfaces
+        // there — far from the cause. Say so here instead.
         if let Err(e) = save_config(&*shared.config, &cfg).await {
             error!(error = %e, "telegram: failed to write pairing to config table");
-        } else {
+            bot.send_message(chat_id, "⚠️ Could not start pairing (the server refused to store the code). Please try again, or ask the admin.")
+                .await.ok();
+            return;
+        }
         // Update the in-memory cache immediately (the config_listener will
         // also fire, but this avoids a race if the user sends another
         // message before the event arrives).
         *shared.bindings.write().await = cfg.clone();
-        }
         info!(chat_id = chat_id.0, code = %code, "TELEGRAM PAIRING: code written to config table");
     }
 
@@ -230,6 +264,32 @@ mod tests {
                 "bindings for other chats are untouched");
     }
 
+    /// A `ConfigApi` over one in-memory value, so the load path can be tested
+    /// without a database.
+    struct FakeConfig(Option<String>);
+
+    #[async_trait::async_trait]
+    impl ConfigApi for FakeConfig {
+        async fn get(&self, _key: &str) -> anyhow::Result<Option<String>> { Ok(self.0.clone()) }
+        async fn set(&self, _key: &str, _value: &str) -> anyhow::Result<()> { Ok(()) }
+    }
+
+    /// The distinction the silent `unwrap_or_default()` used to erase: an absent
+    /// key is a fresh install, an unreadable one must not present itself as an
+    /// empty config that the next write would then persist over the real one.
+    #[tokio::test]
+    async fn an_absent_key_is_empty_and_an_unreadable_one_is_an_error() {
+        let empty = load_config(&FakeConfig(None)).await.unwrap();
+        assert!(empty.bindings.is_empty() && empty.pending_pairings.is_empty());
+
+        let err = load_config(&FakeConfig(Some("{ not json".into()))).await.unwrap_err();
+        assert!(err.to_string().contains("not readable"), "got: {err}");
+
+        // A blob from a future/other schema is unreadable too — `bindings` must
+        // be an array of objects, and a wrong shape has to fail, not default.
+        assert!(load_config(&FakeConfig(Some(r#"{"bindings":"nope"}"#.into()))).await.is_err());
+    }
+
     #[test]
     fn unknown_code_fails_and_keeps_state() {
         let mut cfg = cfg_with_pairing("ABC123", 42);

crates/plugin-telegram-bot/src/lib.rs

@@ -114,6 +114,11 @@ pub(crate) struct TgShared {
     pub(crate) location:     Arc<dyn LocationUpdater>,
 
     // ── Pairing / bindings (config-table-backed, cached in memory) ──
+    /// Hot-path cache for the `chat_id → user_id` lookup every inbound message
+    /// does. Refreshed from the (lossy) `ConfigKeyUpdated` broadcast, so it is
+    /// eventually-consistent by construction: fine for a binding, where a
+    /// dropped event costs one message, and **not** fine for issuing a pairing
+    /// code, which reads the store directly (see `auth::handle_pairing`).
     pub(crate) bindings:     RwLock<auth::TelegramConfig>,
 
     // ── Per-chat pending state ──
@@ -243,7 +248,7 @@ impl Plugin for TelegramPlugin {
         let shared = self.shared()
             .ok_or_else(|| anyhow::anyhow!("telegram: the bot is not running — ask the admin to check the plugin"))?
             .clone();
-        let mut cfg = auth::load_config(&*shared.config).await.unwrap_or_default();
+        let mut cfg = auth::load_config(&*shared.config).await?;
         let chat_id = auth::apply_pairing_code(&mut cfg, code, user_id)?;
         auth::save_config(&*shared.config, &cfg).await?;
         ctx.user_config
@@ -293,9 +298,11 @@ impl Plugin for TelegramPlugin {
             anyhow::bail!("telegram: token is empty — set it via the plugins API");
         }
 
-        // Load bindings from the config table (or default if absent).
+        // Load bindings from the config table (empty if the key is absent). An
-        let telegram_config = auth::load_config(&*ctx.config).await
+        // unreadable blob fails the start on purpose — running with an empty
-            .unwrap_or_default();
+        // cache would hand out pairing codes the store contradicts and let the
+        // first write bury the real bindings.
+        let telegram_config = auth::load_config(&*ctx.config).await?;
         info!(
             bindings = telegram_config.bindings.len(),
             pending   = telegram_config.pending_pairings.len(),

crates/plugin-telegram-bot/src/tools.rs

@@ -311,7 +311,7 @@ impl Tool for TelegramPairingTool {
 
             match action {
                 "list" => {
-                    let cfg = load_config(cfg_api).await.unwrap_or_default();
+                    let cfg = load_config(cfg_api).await?;
                     if cfg.bindings.is_empty() {
                         return Ok("No Telegram bindings.".to_string());
                     }
@@ -327,7 +327,7 @@ impl Tool for TelegramPairingTool {
                         .and_then(Value::as_i64)
                         .ok_or_else(|| anyhow::anyhow!("telegram_pairing: `chat_id` required for unbind"))?;
 
-                    let mut cfg = load_config(cfg_api).await.unwrap_or_default();
+                    let mut cfg = load_config(cfg_api).await?;
                     let before = cfg.bindings.len();
                     cfg.bindings.retain(|b| b.chat_id != chat_id);
                     if cfg.bindings.len() == before {
@@ -338,7 +338,7 @@ impl Tool for TelegramPairingTool {
                 }
 
                 "bind" => {
-                    let mut cfg = load_config(cfg_api).await.unwrap_or_default();
+                    let mut cfg = load_config(cfg_api).await?;
 
                     // Resolve chat_id + user_id either from a pairing code or
                     // from explicit arguments.

crates/skald-core/src/chat_hub/mod.rs

@@ -15,7 +15,7 @@ use inbox::{ConversationInbox, QueuedMessage, build_unit, drain_leading_user};
 
 use crate::approval::ApprovalManager;
 use crate::cron::TaskManager;
-use crate::db::{chat_history, chat_llm_tools, chat_sessions, chat_sessions_stack, config, sources};
+use crate::db::{chat_history, chat_llm_tools, chat_sessions, chat_sessions_stack, sources, user_config};
 use crate::events::{GlobalEvent, ServerEvent};
 use crate::notification::Notification;
 use crate::session::handler::{
@@ -434,15 +434,23 @@ impl ChatHub {
     }
 
     /// Set which source is the "home" for background agent notifications.
+    ///
+    /// The hub is owner-bound, so `self.db` is that person's own database and the
+    /// home is theirs: one member choosing Telegram cannot move anybody else's
+    /// notifications. That is why the key lives in the owner table `user_config`
+    /// and not in the registry `config` one — which this used to write, against a
+    /// `{userid}.db` that has no such table, so `/sethome` only ever answered
+    /// "no such table: config" and every notification batch was dropped by the
+    /// consumer below.
     pub async fn set_home(&self, source_id: &str) -> anyhow::Result<()> {
-        config::set(&self.db, HOME_SOURCE_KEY, source_id).await?;
+        user_config::set(&self.db, HOME_SOURCE_KEY, source_id).await?;
         info!(source_id, "ChatHub: home source set");
         Ok(())
     }
 
     /// Returns the current home source id, falling back to `web` if not configured.
     pub async fn home_source(&self) -> anyhow::Result<String> {
-        Ok(config::get(&self.db, HOME_SOURCE_KEY)
+        Ok(user_config::get(&self.db, HOME_SOURCE_KEY)
             .await?
             .unwrap_or_else(|| DEFAULT_HOME_SOURCE.to_string()))
     }
@@ -978,9 +986,18 @@ impl ChatHub {
                 None    => break, // ChatHub dropped
             };
 
+            // A batch that got this far is data nobody can recreate, and the
+            // destination is the one thing here with a sane default — so a failed
+            // read degrades to it instead of discarding the notifications (which
+            // is precisely what a missing `config` table did, silently, to every
+            // `notify` and every cron completion on the box).
             let home = match hub.home_source().await {
                 Ok(h)  => h,
-                Err(e) => { error!(error = %e, "notification consumer: home_source failed"); continue; }
+                Err(e) => {
+                    error!(error = %e, fallback = DEFAULT_HOME_SOURCE,
+                           "notification consumer: home_source failed");
+                    DEFAULT_HOME_SOURCE.to_string()
+                }
             };
 
             let count = notes.len();

crates/skald-core/src/db/mod.rs

@@ -36,6 +36,7 @@ pub mod system_agent_coverage;
 pub mod system_agent_runs;
 pub mod system_agent_state;
 pub mod tool_permission_groups;
+pub mod user_config;
 pub mod users;
 
 use std::path::{Path, PathBuf};
@@ -1144,6 +1145,23 @@ pub async fn create_owner_tables(pool: &SqlitePool) -> Result<()> {
     .execute(pool)
     .await?;
 
+    // One owner's own preferences — the per-user twin of the registry `config`
+    // table, deliberately **not** sharing its name. The two hold different
+    // namespaces (`ui_locale` and `compaction_model` are the admin's, the home
+    // source is the member's), and a same-named table in both files would turn
+    // every wrong-pool call into a silent read of the other scope instead of the
+    // loud "no such table" that caught `/sethome` writing a per-user setting
+    // through `db::config` against a `{userid}.db`.
+    sqlx::query(
+        "CREATE TABLE IF NOT EXISTS user_config (
+            key        TEXT PRIMARY KEY,
+            value      TEXT NOT NULL,
+            updated_at TEXT NOT NULL DEFAULT (datetime('now'))
+        )",
+    )
+    .execute(pool)
+    .await?;
+
     // NOTE: `projects` + `project_members` are **registry** tables (see
     // `create_registry_tables`) — shareable, not encrypted. The old owner-bucket
     // `projects`/`project_tickets` tables (single-user Skald leftover) were removed
@@ -1337,6 +1355,7 @@ mod tests {
         one("INSERT INTO mcp_events (source, method, payload) VALUES ('s', 'm', '{}')").await.unwrap();
         one("INSERT INTO sources (id, active_session_id) VALUES ('web', 1)").await.unwrap();
         one("INSERT INTO secrets (key, value) VALUES ('k', 'v')").await.unwrap();
+        one("INSERT INTO user_config (key, value) VALUES ('source_home', 'telegram')").await.unwrap();
         one("INSERT INTO llm_request_payloads (request_id, request_json) VALUES ('r1', '{}')").await.unwrap();
         // Fires the AFTER INSERT trigger into the external-content FTS5 table.
         one("INSERT INTO memory_docs (path, content) VALUES ('notes/x.md', 'hello world')").await.unwrap();

crates/skald-core/src/db/user_config.rs

@@ -0,0 +1,46 @@
+//! One owner's own key/value preferences, in their own database.
+//!
+//! The per-user twin of [`super::config`]: same shape, different file and a
+//! different name on purpose (see the table comment in
+//! [`super::create_owner_tables`]). Anything scoped to a person — the surface
+//! their notifications go to, say — belongs here; instance-wide settings the
+//! admin owns stay in the registry `config` table.
+
+use sqlx::SqlitePool;
+
+/// Get a value by key from this owner's database.
+pub async fn get(pool: &SqlitePool, key: &str) -> anyhow::Result<Option<String>> {
+    let row = sqlx::query_as::<_, (String,)>(
+        "SELECT value FROM user_config WHERE key = ?",
+    )
+    .bind(key)
+    .fetch_optional(pool)
+    .await?;
+
+    Ok(row.map(|(v,)| v))
+}
+
+/// Upsert a key/value pair in this owner's database.
+pub async fn set(pool: &SqlitePool, key: &str, value: &str) -> anyhow::Result<()> {
+    sqlx::query(
+        "INSERT INTO user_config (key, value, updated_at)
+         VALUES (?, ?, datetime('now'))
+         ON CONFLICT(key) DO UPDATE SET
+             value      = excluded.value,
+             updated_at = excluded.updated_at",
+    )
+    .bind(key)
+    .bind(value)
+    .execute(pool)
+    .await?;
+    Ok(())
+}
+
+/// Delete an entry.
+pub async fn delete(pool: &SqlitePool, key: &str) -> anyhow::Result<()> {
+    sqlx::query("DELETE FROM user_config WHERE key = ?")
+        .bind(key)
+        .execute(pool)
+        .await?;
+    Ok(())
+}