From 08a5f46be184340247d70a7aaa4a31c684c5ed38 Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Sun, 10 May 2026 14:03:41 -0400 Subject: [PATCH] chat: scope insight lookup by library_id to fix regen-shadow bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a photo exists in more than one library and the user regenerates its insight from library A's chat, the regenerate streams cleanly, store_insight flips library A's old row to is_current=false, and inserts a new is_current=true row tagged (library A, rel_path). On the next history fetch the user sees their old transcript — the regenerate appears to vanish. The cause: get_insight(file_path) filters on rel_path + is_current only, so library B's untouched is_current=true row for the same rel_path satisfies the query and gets returned by SQLite's .first() ahead of A's new row. Because get_insight is also what chat_turn_stream uses to decide bootstrap vs. continuation, the next chat turn after the shadow hit also routes against the wrong insight, so update_training_messages corrupts library B's transcript with library A's chat. Fix: add get_current_insight_for_library(library_id, file_path) filtered on (library_id, rel_path, is_current=true) and route the chat surface (load_history, chat_turn{,_stream}, rewind_history) through it. load_history falls back to the cross-library get_insight when the scoped lookup misses — preserves the "scalar data merges across libraries" intent for the case where the active library has no insight but another does. The path-only get_insight stays for callers that don't have library context (populate_knowledge, the photo-grid metadata fetch). chat_history_handler stops dropping the parsed library on the floor and threads it through. Single-library deploys see no behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/ai/handlers.rs | 16 ++++++++++----- src/ai/insight_chat.rs | 40 ++++++++++++++++++++++++++---------- src/database/insights_dao.rs | 38 ++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 16 deletions(-) diff --git a/src/ai/handlers.rs b/src/ai/handlers.rs index f296989..f6189ba 100644 --- a/src/ai/handlers.rs +++ b/src/ai/handlers.rs @@ -874,15 +874,21 @@ pub async fn chat_history_handler( query: web::Query, app_state: web::Data, ) -> impl Responder { - // library param parsed for parity with other insight endpoints, even - // though load_history currently keys on file_path alone (matches the - // existing get_insight DAO contract). - let _library = libraries::resolve_library_param(&app_state, query.library.as_deref()) + // library_id scopes the lookup so a regenerate on this library + // isn't shadowed by an untouched is_current=true row in another + // library for the same rel_path. load_history falls back to the + // cross-library lookup when the scoped one misses, so a photo + // with no insight in this library but one in another still + // surfaces (the "show this photo's primary insight" merge case). + let library = libraries::resolve_library_param(&app_state, query.library.as_deref()) .ok() .flatten() .unwrap_or_else(|| app_state.primary_library()); - match app_state.insight_chat.load_history(&query.path) { + match app_state + .insight_chat + .load_history(library.id, &query.path) + { Ok(view) => HttpResponse::Ok().json(ChatHistoryHttpResponse { messages: view .messages diff --git a/src/ai/insight_chat.rs b/src/ai/insight_chat.rs index 64918f5..5a0e3f9 100644 --- a/src/ai/insight_chat.rs +++ b/src/ai/insight_chat.rs @@ -118,14 +118,28 @@ impl InsightChatService { /// scaffolding (system message, tool turns, tool-dispatch-only assistant /// messages) and drops base64 images from user turns to keep payloads /// small. The first remaining user message is flagged `is_initial`. - pub fn load_history(&self, file_path: &str) -> Result { + /// + /// `library_id` scopes the lookup to one library — without it, a + /// regenerate on lib1 can be shadowed on the next refresh by an + /// untouched `is_current=true` row in lib2 for the same rel_path. + /// Falls back to the cross-library `get_insight` only when the + /// scoped lookup misses, preserving the cross-library "show this + /// photo's primary insight" merge for the case where the active + /// library has no insight but another library does. + pub fn load_history(&self, library_id: i32, file_path: &str) -> Result { let normalized = normalize_path(file_path); let cx = opentelemetry::Context::new(); let mut dao = self.insight_dao.lock().expect("Unable to lock InsightDao"); - let insight = dao - .get_insight(&cx, &normalized) + let insight = match dao + .get_current_insight_for_library(&cx, library_id, &normalized) .map_err(|e| anyhow!("failed to load insight: {:?}", e))? - .ok_or_else(|| anyhow!("no insight found for path"))?; + { + Some(i) => i, + None => dao + .get_insight(&cx, &normalized) + .map_err(|e| anyhow!("failed to load insight: {:?}", e))? + .ok_or_else(|| anyhow!("no insight found for path"))?, + }; let raw = insight .training_messages @@ -267,7 +281,7 @@ impl InsightChatService { let insight = { let cx = opentelemetry::Context::new(); let mut dao = self.insight_dao.lock().expect("Unable to lock InsightDao"); - dao.get_insight(&cx, &normalized) + dao.get_current_insight_for_library(&cx, req.library_id, &normalized) .map_err(|e| anyhow!("failed to load insight: {:?}", e))? .ok_or_else(|| anyhow!("no insight found for path"))? }; @@ -627,7 +641,7 @@ impl InsightChatService { let insight = { let cx = opentelemetry::Context::new(); let mut dao = self.insight_dao.lock().expect("Unable to lock InsightDao"); - dao.get_insight(&cx, &normalized) + dao.get_current_insight_for_library(&cx, library_id, &normalized) .map_err(|e| anyhow!("failed to load insight: {:?}", e))? .ok_or_else(|| anyhow!("no insight found for path"))? }; @@ -726,14 +740,18 @@ impl InsightChatService { }; let _guard = entry_lock.lock().await; - // Look up existing insight (None when missing). The branch below - // decides bootstrap vs. continuation: `regenerate=true` forces - // bootstrap regardless of the row's presence; missing rows always - // bootstrap. + // Look up existing insight scoped to this turn's library_id. + // Path-only lookup would let an `is_current=true` row in + // another library route us into the continuation path against + // a transcript we'd then update_training_messages on — corrupting + // the other library's curated insight. Library-scoped lookup + // means a fresh chat on a photo that has no insight in this + // library bootstraps cleanly, even when another library has + // an insight for the same rel_path. let existing_insight = { let cx = opentelemetry::Context::new(); let mut dao = self.insight_dao.lock().expect("Unable to lock InsightDao"); - dao.get_insight(&cx, &normalized) + dao.get_current_insight_for_library(&cx, req.library_id, &normalized) .map_err(|e| anyhow!("failed to load insight: {:?}", e))? }; diff --git a/src/database/insights_dao.rs b/src/database/insights_dao.rs index 28de5f4..8c7551c 100644 --- a/src/database/insights_dao.rs +++ b/src/database/insights_dao.rs @@ -21,6 +21,22 @@ pub trait InsightDao: Sync + Send { file_path: &str, ) -> Result, DbError>; + /// Library-scoped variant of `get_insight`. The default `get_insight` + /// finds any `is_current=true` row matching `file_path` across + /// libraries — fine for the photo-grid metadata fetch (cross-library + /// merge), wrong for the chat path: a regenerate on lib1 flips lib1's + /// row to `is_current=false` and inserts a new lib1 row, but + /// lib2's untouched `is_current=true` row for the same rel_path + /// would still satisfy the path-only query and shadow the regen on + /// the next history fetch. Always pass a library_id when you have + /// one (chat / insight write paths always do). + fn get_current_insight_for_library( + &mut self, + context: &opentelemetry::Context, + library_id: i32, + file_path: &str, + ) -> Result, DbError>; + /// Return the most recent current insight whose rel_path is one of /// `paths`. Used for content-hash sharing: the caller expands a /// single file into all rel_paths with the same content_hash, then @@ -182,6 +198,28 @@ impl InsightDao for SqliteInsightDao { .map_err(|_| DbError::new(DbErrorKind::QueryError)) } + fn get_current_insight_for_library( + &mut self, + context: &opentelemetry::Context, + lib_id: i32, + path: &str, + ) -> Result, DbError> { + trace_db_call(context, "query", "get_current_insight_for_library", |_span| { + use schema::photo_insights::dsl::*; + + let mut connection = self.connection.lock().expect("Unable to get InsightDao"); + + photo_insights + .filter(library_id.eq(lib_id)) + .filter(rel_path.eq(path)) + .filter(is_current.eq(true)) + .first::(connection.deref_mut()) + .optional() + .map_err(|_| anyhow::anyhow!("Query error")) + }) + .map_err(|_| DbError::new(DbErrorKind::QueryError)) + } + fn get_insight_for_paths( &mut self, context: &opentelemetry::Context,