From e539c083c93cff528095921f95f1794c194cfc1b Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Thu, 7 May 2026 15:07:57 -0400 Subject: [PATCH] insight-chat: code-review polish on the tool-gating PR - search_messages now delegates to search_messages_with_contact(.., None) so the two methods share a single HTTP path. Drops the dead-code warning and the ~30-line duplication. - DailySummaryDao gains has_any_summaries (LIMIT 1 existence probe) used by current_gate_opts; the SELECT COUNT(*) get_total_summary_count added in the prior commit is removed (it had no other caller). - current_gate_opts doc comment corrected to describe what the probes actually do. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/ai/insight_generator.rs | 15 +++++++------- src/ai/sms_client.rs | 30 ++++----------------------- src/database/daily_summary_dao.rs | 34 +++++++++++++++++++------------ 3 files changed, 33 insertions(+), 46 deletions(-) diff --git a/src/ai/insight_generator.rs b/src/ai/insight_generator.rs index 1cba095..98995e3 100644 --- a/src/ai/insight_generator.rs +++ b/src/ai/insight_generator.rs @@ -144,10 +144,13 @@ impl InsightGenerator { } /// Compute the per-call tool gate options by probing each backing - /// table. Cheap (`SELECT 1 FROM LIMIT 1` shape via the existing - /// count methods); meant to be called once per chat turn / generation. - /// `has_vision` is supplied by the caller because it depends on the - /// model selected for this turn, not on persistent state. + /// table for presence. `daily_summaries_present` uses a `LIMIT 1` + /// existence probe; `calendar_present` and `location_history_present` + /// use the existing `get_event_count` / `get_location_count` + /// methods (small enough that a full `COUNT(*)` is fine). Meant to + /// be called once per chat turn / generation. `has_vision` is + /// supplied by the caller because it depends on the model selected + /// for this turn, not on persistent state. pub fn current_gate_opts(&self, has_vision: bool) -> ToolGateOpts { let cx = opentelemetry::Context::new(); let calendar_present = { @@ -169,9 +172,7 @@ impl InsightGenerator { .daily_summary_dao .lock() .expect("Unable to lock DailySummaryDao"); - dao.get_total_summary_count(&cx) - .map(|n| n > 0) - .unwrap_or(false) + dao.has_any_summaries(&cx).unwrap_or(false) }; ToolGateOpts { has_vision, diff --git a/src/ai/sms_client.rs b/src/ai/sms_client.rs index 1ead9df..cddac00 100644 --- a/src/ai/sms_client.rs +++ b/src/ai/sms_client.rs @@ -261,38 +261,16 @@ impl SmsApiClient { /// - "fts5" keyword-only, supports phrase / prefix / boolean / NEAR /// - "semantic" embedding similarity /// - "hybrid" both merged via reciprocal rank fusion (recommended) + /// + /// Equivalent to `search_messages_with_contact(query, mode, limit, None)`; + /// kept as a convenience for callers that don't filter by contact. pub async fn search_messages( &self, query: &str, mode: &str, limit: usize, ) -> Result> { - let url = format!( - "{}/api/messages/search/?q={}&mode={}&limit={}", - self.base_url, - urlencoding::encode(query), - urlencoding::encode(mode), - limit - ); - - let mut request = self.client.get(&url); - if let Some(token) = &self.token { - request = request.header("Authorization", format!("Bearer {}", token)); - } - - let response = request.send().await?; - if !response.status().is_success() { - let status = response.status(); - let body = response.text().await.unwrap_or_default(); - return Err(anyhow::anyhow!( - "SMS search request failed: {} - {}", - status, - body - )); - } - - let data: SmsSearchResponse = response.json().await?; - Ok(data.results) + self.search_messages_with_contact(query, mode, limit, None).await } /// Same shape as `search_messages` but with optional `contact_id`. The diff --git a/src/database/daily_summary_dao.rs b/src/database/daily_summary_dao.rs index 5614a06..8f1c5a9 100644 --- a/src/database/daily_summary_dao.rs +++ b/src/database/daily_summary_dao.rs @@ -76,12 +76,13 @@ pub trait DailySummaryDao: Sync + Send { contact: &str, ) -> Result; - /// Get total count of all summaries (across all contacts). Used by - /// `current_gate_opts` to check whether daily_summaries are present. - fn get_total_summary_count( + /// Cheap presence check — returns true iff at least one daily summary row + /// exists. Used by gating logic that only needs "is the table empty?", + /// avoiding a `COUNT(*)` full scan on large corpora. + fn has_any_summaries( &mut self, context: &opentelemetry::Context, - ) -> Result; + ) -> Result; } pub struct SqliteDailySummaryDao { @@ -462,20 +463,27 @@ impl DailySummaryDao for SqliteDailySummaryDao { .map_err(|_| DbError::new(DbErrorKind::QueryError)) } - fn get_total_summary_count( - &mut self, - context: &opentelemetry::Context, - ) -> Result { - trace_db_call(context, "query", "get_total_summary_count", |_span| { + fn has_any_summaries(&mut self, context: &opentelemetry::Context) -> Result { + trace_db_call(context, "query", "has_any_summaries", |_span| { let mut conn = self .connection .lock() .expect("Unable to get DailySummaryDao"); - diesel::sql_query("SELECT COUNT(*) as count FROM daily_conversation_summaries") - .get_result::(conn.deref_mut()) - .map(|r| r.count) - .map_err(|e| anyhow::anyhow!("Count query error: {:?}", e)) + #[derive(QueryableByName)] + struct ProbeResult { + #[diesel(sql_type = diesel::sql_types::Integer)] + #[allow(dead_code)] + one: i32, + } + + let rows: Vec = diesel::sql_query( + "SELECT 1 as one FROM daily_conversation_summaries LIMIT 1", + ) + .load(conn.deref_mut()) + .map_err(|e| anyhow::anyhow!("Failed to probe daily summaries: {}", e))?; + + Ok(!rows.is_empty()) }) .map_err(|_| DbError::new(DbErrorKind::QueryError)) }