fix: address PR review feedback for libSQL backend

- P0: Switch libsql_backend to connection-per-operation pattern to fix
  shared Connection concurrency issue across tokio tasks
- P0: Wrap secrets store INSERT+SELECT in transaction to fix TOCTOU race
- P0: Document encryption-at-rest limitations and json_patch divergence
- P1: Fix get_opt_text removing .filter(|s| !s.is_empty()) that conflated
  empty strings with NULL
- P1: Replace datetime('now') with fmt_ts(&Utc::now()) for consistent
  RFC 3339 timestamps across all queries
- P2: Use explicit _rowid column in FTS5 triggers and joins for stability
  across VACUUM operations
- P2: Add tracing::warn when embedding provided but vector search disabled
  in hybrid_search
- Extract shared connect_from_config() helper to deduplicate DB connection
  logic across main.rs, cli/config.rs, and cli/mcp.rs

Co-Authored-By: Claude Opus 4.6 <[email protected]>

CLAUDE.md

@@ -409,6 +409,8 @@ LIBSQL_AUTH_TOKEN=your-token            # Required when LIBSQL_URL is set
 - **Hybrid search** uses FTS5 only (vector search via libsql_vector_idx not yet implemented)
 - **Settings reload from DB** skipped (Config::from_db requires Store)
 - No incremental migration versioning (schema is CREATE IF NOT EXISTS, no ALTER TABLE support yet)
+- **No encryption at rest** -- The local SQLite database file stores conversation content, job data, workspace memory, and other application data in plaintext. Only secrets (API tokens, credentials) are encrypted via AES-256-GCM before storage. Users handling sensitive data should use full-disk encryption (FileVault, LUKS, BitLocker) or consider the PostgreSQL backend with TDE/encrypted storage.
+- **JSON merge patch vs path-targeted update** -- The libSQL backend uses RFC 7396 JSON Merge Patch (`json_patch`) for metadata updates, while PostgreSQL uses path-targeted `jsonb_set`. Merge patch replaces top-level keys entirely, which may drop nested keys not present in the patch. Callers should avoid relying on partial nested object updates in metadata fields.
 
 ## Safety Layer
 

src/cli/config.rs

@@ -3,6 +3,8 @@
 //! Commands for viewing and modifying settings.
 //! Settings are stored in the database (env > DB > default).
 
+use std::sync::Arc;
+
 use clap::Subcommand;
 
 use crate::settings::Settings;
@@ -49,7 +51,7 @@ pub async fn run_config_command(cmd: ConfigCommand) -> anyhow::Result<()> {
     let _ = dotenvy::dotenv();
 
     // Try to connect to the DB for settings access
-    let db: Option<Box<dyn crate::db::Database>> = match connect_db().await {
+    let db: Option<Arc<dyn crate::db::Database>> = match connect_db().await {
         Ok(d) => Some(d),
         Err(e) => {
             eprintln!(
@@ -71,38 +73,11 @@ pub async fn run_config_command(cmd: ConfigCommand) -> anyhow::Result<()> {
 }
 
 /// Bootstrap a DB connection for config commands (backend-agnostic).
-async fn connect_db() -> anyhow::Result<Box<dyn crate::db::Database>> {
-    use crate::db::Database as _;
+async fn connect_db() -> anyhow::Result<Arc<dyn crate::db::Database>> {
     let config = crate::config::Config::from_env().await.map_err(|e| anyhow::anyhow!("{}", e))?;
-
-    match config.database.backend {
-        #[cfg(feature = "libsql")]
-        crate::config::DatabaseBackend::LibSql => {
-            use secrecy::ExposeSecret as _;
-            let default_path = crate::config::default_libsql_path();
-            let db_path = config.database.libsql_path.as_deref().unwrap_or(&default_path);
-            let backend = if let Some(ref url) = config.database.libsql_url {
-                let token = config.database.libsql_auth_token.as_ref()
-                    .ok_or_else(|| anyhow::anyhow!("LIBSQL_AUTH_TOKEN required when LIBSQL_URL is set"))?;
-                crate::db::libsql_backend::LibSqlBackend::new_remote_replica(db_path, url, token.expose_secret()).await?
-            } else {
-                crate::db::libsql_backend::LibSqlBackend::new_local(db_path).await?
-            };
-            backend.run_migrations().await?;
-            Ok(Box::new(backend))
-        }
-        #[cfg(feature = "postgres")]
-        _ => {
-            let pg = crate::db::postgres::PgBackend::new(&config.database).await
-                .map_err(|e| anyhow::anyhow!("{}", e))?;
-            pg.run_migrations().await.map_err(|e| anyhow::anyhow!("{}", e))?;
-            Ok(Box::new(pg))
-        }
-        #[cfg(not(feature = "postgres"))]
-        _ => {
-            anyhow::bail!("No database backend available. Enable 'postgres' or 'libsql' feature.");
-        }
-    }
+    crate::db::connect_from_config(&config.database)
+        .await
+        .map_err(|e| anyhow::anyhow!("{}", e))
 }
 
 const DEFAULT_USER_ID: &str = "default";

src/cli/mcp.rs

@@ -468,33 +468,9 @@ async fn toggle_server(name: String, enable: bool, disable: bool) -> anyhow::Res
 const DEFAULT_USER_ID: &str = "default";
 
 /// Try to connect to the database (backend-agnostic).
-async fn connect_db() -> Option<Box<dyn Database>> {
-    use crate::db::Database as _;
+async fn connect_db() -> Option<Arc<dyn Database>> {
     let config = Config::from_env().await.ok()?;
-    match config.database.backend {
-        #[cfg(feature = "libsql")]
-        crate::config::DatabaseBackend::LibSql => {
-            use secrecy::ExposeSecret as _;
-            let default_path = crate::config::default_libsql_path();
-            let db_path = config.database.libsql_path.as_deref().unwrap_or(&default_path);
-            let backend = if let Some(ref url) = config.database.libsql_url {
-                let token = config.database.libsql_auth_token.as_ref()?;
-                crate::db::libsql_backend::LibSqlBackend::new_remote_replica(db_path, url, token.expose_secret()).await.ok()?
-            } else {
-                crate::db::libsql_backend::LibSqlBackend::new_local(db_path).await.ok()?
-            };
-            backend.run_migrations().await.ok()?;
-            Some(Box::new(backend))
-        }
-        #[cfg(feature = "postgres")]
-        _ => {
-            let pg = crate::db::postgres::PgBackend::new(&config.database).await.ok()?;
-            pg.run_migrations().await.ok()?;
-            Some(Box::new(pg))
-        }
-        #[cfg(not(feature = "postgres"))]
-        _ => None,
-    }
+    crate::db::connect_from_config(&config.database).await.ok()
 }
 
 /// Load MCP servers (DB if available, else disk).

src/db/libsql_migrations.rs

@@ -216,7 +216,8 @@ CREATE TRIGGER IF NOT EXISTS update_memory_documents_updated_at
 -- ==================== Workspace: Memory Chunks ====================
 
 CREATE TABLE IF NOT EXISTS memory_chunks (
-    id TEXT PRIMARY KEY,
+    _rowid INTEGER PRIMARY KEY AUTOINCREMENT,
+    id TEXT NOT NULL UNIQUE,
     document_id TEXT NOT NULL REFERENCES memory_documents(id) ON DELETE CASCADE,
     chunk_index INTEGER NOT NULL,
     content TEXT NOT NULL,
@@ -234,24 +235,24 @@ CREATE INDEX IF NOT EXISTS idx_memory_chunks_embedding
 -- FTS5 virtual table for full-text search
 CREATE VIRTUAL TABLE IF NOT EXISTS memory_chunks_fts USING fts5(
     content,
-    content=memory_chunks,
-    content_rowid=rowid
+    content='memory_chunks',
+    content_rowid='_rowid'
 );
 
 -- Triggers to keep FTS5 in sync with memory_chunks
 CREATE TRIGGER IF NOT EXISTS memory_chunks_fts_insert AFTER INSERT ON memory_chunks BEGIN
-    INSERT INTO memory_chunks_fts(rowid, content) VALUES (new.rowid, new.content);
+    INSERT INTO memory_chunks_fts(rowid, content) VALUES (new._rowid, new.content);
 END;
 
 CREATE TRIGGER IF NOT EXISTS memory_chunks_fts_delete AFTER DELETE ON memory_chunks BEGIN
     INSERT INTO memory_chunks_fts(memory_chunks_fts, rowid, content)
-        VALUES ('delete', old.rowid, old.content);
+        VALUES ('delete', old._rowid, old.content);
 END;
 
 CREATE TRIGGER IF NOT EXISTS memory_chunks_fts_update AFTER UPDATE ON memory_chunks BEGIN
     INSERT INTO memory_chunks_fts(memory_chunks_fts, rowid, content)
-        VALUES ('delete', old.rowid, old.content);
-    INSERT INTO memory_chunks_fts(rowid, content) VALUES (new.rowid, new.content);
+        VALUES ('delete', old._rowid, old.content);
+    INSERT INTO memory_chunks_fts(rowid, content) VALUES (new._rowid, new.content);
 END;
 
 -- ==================== Workspace: Heartbeat State ====================

src/db/mod.rs

@@ -19,6 +19,7 @@ pub mod libsql_backend;
 pub mod libsql_migrations;
 
 use std::collections::HashMap;
+use std::sync::Arc;
 
 use async_trait::async_trait;
 use chrono::{DateTime, Utc};
@@ -37,6 +38,60 @@ use crate::history::{
 use crate::workspace::{MemoryChunk, MemoryDocument, WorkspaceEntry};
 use crate::workspace::{SearchConfig, SearchResult};
 
+/// Create a database backend from configuration, run migrations, and return it.
+///
+/// This is the shared helper for CLI commands and other call sites that need
+/// a simple `Arc<dyn Database>` without retaining backend-specific handles
+/// (e.g., `pg_pool` or `libsql_conn` for the secrets store). The main agent
+/// startup in `main.rs` uses its own initialization block because it also
+/// captures those backend-specific handles.
+pub async fn connect_from_config(
+    config: &crate::config::DatabaseConfig,
+) -> Result<Arc<dyn Database>, DatabaseError> {
+    match config.backend {
+        #[cfg(feature = "libsql")]
+        crate::config::DatabaseBackend::LibSql => {
+            use secrecy::ExposeSecret as _;
+
+            let default_path = crate::config::default_libsql_path();
+            let db_path = config.libsql_path.as_deref().unwrap_or(&default_path);
+
+            let backend = if let Some(ref url) = config.libsql_url {
+                let token = config.libsql_auth_token.as_ref().ok_or_else(|| {
+                    DatabaseError::Pool(
+                        "LIBSQL_AUTH_TOKEN required when LIBSQL_URL is set".to_string(),
+                    )
+                })?;
+                libsql_backend::LibSqlBackend::new_remote_replica(
+                    db_path,
+                    url,
+                    token.expose_secret(),
+                )
+                .await
+                .map_err(|e| DatabaseError::Pool(e.to_string()))?
+            } else {
+                libsql_backend::LibSqlBackend::new_local(db_path)
+                    .await
+                    .map_err(|e| DatabaseError::Pool(e.to_string()))?
+            };
+            backend.run_migrations().await?;
+            Ok(Arc::new(backend))
+        }
+        #[cfg(feature = "postgres")]
+        _ => {
+            let pg = postgres::PgBackend::new(config)
+                .await
+                .map_err(|e| DatabaseError::Pool(e.to_string()))?;
+            pg.run_migrations().await?;
+            Ok(Arc::new(pg))
+        }
+        #[cfg(not(feature = "postgres"))]
+        _ => Err(DatabaseError::Pool(
+            "No database backend available. Enable 'postgres' or 'libsql' feature.".to_string(),
+        )),
+    }
+}
+
 /// Backend-agnostic database trait.
 ///
 /// Combines all persistence operations from Store, Repository, and related

src/main.rs

@@ -133,40 +133,10 @@ async fn main() -> anyhow::Result<()> {
                 };
 
             // Create a Database-trait-backed workspace for the memory command
-            let db: Arc<dyn ironclaw::db::Database> = match config.database.backend {
-                #[cfg(feature = "libsql")]
-                ironclaw::config::DatabaseBackend::LibSql => {
-                    use ironclaw::db::libsql_backend::LibSqlBackend;
-                    use ironclaw::db::Database as _;
-                    use secrecy::ExposeSecret as _;
-
-                    let default_path = ironclaw::config::default_libsql_path();
-                    let db_path = config.database.libsql_path.as_deref()
-                        .unwrap_or(&default_path);
-
-                    let backend = if let Some(ref url) = config.database.libsql_url {
-                        let token = config.database.libsql_auth_token.as_ref()
-                            .expect("LIBSQL_AUTH_TOKEN required when LIBSQL_URL is set");
-                        LibSqlBackend::new_remote_replica(db_path, url, token.expose_secret()).await?
-                    } else {
-                        LibSqlBackend::new_local(db_path).await?
-                    };
-                    backend.run_migrations().await?;
-                    Arc::new(backend)
-                }
-                #[cfg(feature = "postgres")]
-                _ => {
-                    use ironclaw::db::Database as _;
-                    let pg = ironclaw::db::postgres::PgBackend::new(&config.database).await
-                        .map_err(|e| anyhow::anyhow!("{}", e))?;
-                    pg.run_migrations().await.map_err(|e| anyhow::anyhow!("{}", e))?;
-                    Arc::new(pg)
-                }
-                #[cfg(not(feature = "postgres"))]
-                _ => {
-                    anyhow::bail!("No database backend available. Enable 'postgres' or 'libsql' feature.");
-                }
-            };
+            let db: Arc<dyn ironclaw::db::Database> =
+                ironclaw::db::connect_from_config(&config.database)
+                    .await
+                    .map_err(|e| anyhow::anyhow!("{}", e))?;
 
             return ironclaw::cli::run_memory_command_with_db(mem_cmd.clone(), db, embeddings).await;
         }
@@ -370,6 +340,11 @@ async fn main() -> anyhow::Result<()> {
     //
     // Creates an `Arc<dyn Database>` that all consumers share.
     // Backend is selected by the `DATABASE_BACKEND` env var / config.
+    //
+    // NOTE: For simpler call sites (CLI commands, Memory handler) use the shared
+    // helper `ironclaw::db::connect_from_config()`. This block is kept inline
+    // because it also captures backend-specific handles (`pg_pool`, `libsql_conn`)
+    // needed by the secrets store.
     #[cfg(feature = "postgres")]
     let mut pg_pool: Option<deadpool_postgres::Pool> = None;
     #[cfg(feature = "libsql")]

src/secrets/store.rs

@@ -339,8 +339,11 @@ impl SecretsStore for LibSqlSecretsStore {
             .expires_at
             .map(|dt| dt.to_rfc3339_opts(chrono::SecondsFormat::Millis, true));
 
-        self.conn
-            .execute(
+        // Start transaction for atomic upsert + read-back
+        let tx = self.conn.transaction().await
+            .map_err(|e| SecretError::Database(e.to_string()))?;
+
+        tx.execute(
                 r#"
                 INSERT INTO secrets (id, user_id, name, encrypted_value, key_salt, provider, expires_at, created_at, updated_at)
                 VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?8)
@@ -366,8 +369,7 @@ impl SecretsStore for LibSqlSecretsStore {
             .map_err(|e| SecretError::Database(e.to_string()))?;
 
         // Read back the row (may have been upserted)
-        let mut rows = self
-            .conn
+        let mut rows = tx
             .query(
                 r#"
                 SELECT id, user_id, name, encrypted_value, key_salt, provider, expires_at,
@@ -386,7 +388,12 @@ impl SecretsStore for LibSqlSecretsStore {
             .map_err(|e| SecretError::Database(e.to_string()))?
             .ok_or_else(|| SecretError::Database("Insert succeeded but row not found".into()))?;
 
-        libsql_row_to_secret(&row)
+        let secret = libsql_row_to_secret(&row)?;
+
+        tx.commit().await
+            .map_err(|e| SecretError::Database(e.to_string()))?;
+
+        Ok(secret)
     }
 
     async fn get(&self, user_id: &str, name: &str) -> Result<Secret, SecretError> {