From e029c81274814e62381cf6b30c5b7b2db8a137ee Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Tue, 20 Oct 2026 19:38:08 -0400 Subject: [PATCH] fix(persona-chat): review fixes before merge - Thread tools are bound server-side to the chat's own thread. The model used to pass contact_id, so practice mode could read any conversation it named; a model-supplied contact_id is now only checked, never trusted, and the prompts no longer hand it one. - Practice chats need no persona. They're stored under PRACTICE_PERSONA_ID and run with an inert stand-in, so deleting or editing personas can't hide or break them. persona_id is optional on create for as_contact. - The per-thread list filters in SQL (list_persona_chats_for_contact, served by idx_persona_chat_anchor_contact) instead of decoding every transcript. - fetch_mms_image checks the content type and a 20 MB cap before reading the body, so a video attachment isn't downloaded in full. - get_photos_near_time unions tags over every path sharing the content, per the multi-library model. - look_at_photo only opens indexed photos, so an excluded folder can't be read by guessing a path. - One date parser: get_photos_near_time uses date_info::parse_center. - Practice prompt source rewrapped. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 20 +++-- src/ai/date_info.rs | 29 ++++++- src/ai/insight_chat.rs | 5 ++ src/ai/insight_generator.rs | 160 +++++++++++++++++++++--------------- src/ai/persona_chat.rs | 99 ++++++++++++++++++---- src/ai/sms_anchor.rs | 55 ++++++++----- src/ai/sms_client.rs | 71 ++++++++++++++-- src/database/persona_dao.rs | 60 ++++++++++++++ 8 files changed, 383 insertions(+), 116 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0177473..b5d8423 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -578,13 +578,19 @@ GET /insights/chat/history?path=... (rendered transcript with tool invocations) POST /insights/chat/rewind (truncate transcript at a rendered index) // Persona chat — "Continue with persona" from an SMS thread (src/ai/sms_anchor.rs) -POST /persona_chat/conversations (body: { persona_id, anchor?: { contact_id, message_id } }; - anchor snapshots ~40 SMS-API messages up to message_id into - the row, 502 if SMS-API can't read the thread) -GET /persona_chat/conversations?contact_id=N (only chats anchored on that thread) -// Anchored turns append the thread + chat-register guidance to the persona prompt and add -// the get_thread_messages / look_at_message_image / get_photos_near_time / look_at_photo -// tools (ToolGateOpts.sms_thread_tools). Image tools use the LOCAL vision model. +POST /persona_chat/conversations (body: { persona_id, anchor?: { contact_id, message_id, + mode?: "about" | "as_contact" } }; anchor snapshots ~40 + SMS-API messages up to message_id into the row, 502 if + SMS-API can't read the thread. `as_contact` (practice) needs + no persona: stored under PRACTICE_PERSONA_ID, listed as + "Practice") +GET /persona_chat/conversations?contact_id=N (only chats anchored on that thread; DAO query) +// Anchored turns append the thread + mode guidance to the prompt and add the thread +// tools (ToolGateOpts.sms_thread_tools): get_thread_messages / look_at_message_image are +// bound server-side to the chat's own thread (a model-named contact_id is refused), and +// about-mode also gets get_photos_near_time / look_at_photo. Practice keeps only the two +// thread tools. Image tools use the LOCAL vision model; MMS downloads are image-only and +// capped (MMS_IMAGE_MAX_BYTES); look_at_photo only opens indexed photos. // User AI Prefs (owner defaults + passive mirror; src/prefs.rs) GET /prefs (current row; auth) diff --git a/src/ai/date_info.rs b/src/ai/date_info.rs index e6f79e0..289d798 100644 --- a/src/ai/date_info.rs +++ b/src/ai/date_info.rs @@ -10,7 +10,7 @@ //! Everything is computed in the owner's display zone, so the dates match //! the timestamps the model reads in thread transcripts and photo metadata. -use chrono::{DateTime, Datelike, Months, NaiveDate, NaiveDateTime, Utc}; +use chrono::{DateTime, Datelike, Months, NaiveDate, NaiveDateTime, TimeZone, Utc}; use chrono_tz::Tz; /// A date argument as the model gave it. A bare date stays a date (no @@ -63,6 +63,19 @@ fn parse_moment(value: &serde_json::Value, zone: Tz) -> Option { .map(Moment::Day) } +/// A moment for tools that search *around* a time (`get_photos_near_time`), +/// as unix seconds. Same formats as `date_info`; a bare date centres on +/// local midday, the least-wrong centre for a whole day. +pub fn parse_center(value: &serde_json::Value, zone: Tz) -> Option { + let local = match parse_moment(value, zone)? { + Moment::At(dt) => dt, + Moment::Day(d) => d.and_hms_opt(12, 0, 0)?, + }; + zone.from_local_datetime(&local) + .earliest() + .map(|dt| dt.timestamp()) +} + /// Whole years, months and days from `a` to `b` (`a <= b`), counted the /// calendar way: Jan 31 + 1 month is Feb 28/29, as people count ages. fn calendar_gap(a: NaiveDate, b: NaiveDate) -> (u32, u32, u32) { @@ -227,7 +240,6 @@ pub fn date_info(args: &serde_json::Value, now: DateTime, zone: Tz) -> Stri #[cfg(test)] mod tests { use super::*; - use chrono::TimeZone; use serde_json::json; const NY: Tz = chrono_tz::America::New_York; @@ -339,6 +351,19 @@ mod tests { assert_eq!(describe_gap(d(2025, 10, 18), d(2026, 10, 18)), "1 year"); } + #[test] + fn parse_center_reads_unix_wall_clock_and_bare_dates() { + assert_eq!(parse_center(&json!(1_700_000_000), NY), Some(1_700_000_000)); + // 2023-11-14 17:13 in New York (EST) = 22:13 UTC. + assert_eq!( + parse_center(&json!("2023-11-14 17:13"), NY), + Some(1_699_999_980) + ); + // A bare date centres on local midday. + assert_eq!(parse_center(&json!("2023-11-14"), NY), Some(1_699_981_200)); + assert_eq!(parse_center(&json!("soon"), NY), None); + } + #[test] fn bad_input_says_what_format_to_use() { let out = date_info(&json!({"date": "last tuesday"}), now(), NY); diff --git a/src/ai/insight_chat.rs b/src/ai/insight_chat.rs index 1cc329e..3d384dd 100644 --- a/src/ai/insight_chat.rs +++ b/src/ai/insight_chat.rs @@ -473,6 +473,7 @@ impl InsightChatService { &normalized, req.user_id, &active_persona, + None, ) .instrument(loop_span.clone()) .await; @@ -1435,6 +1436,7 @@ impl InsightChatService { normalized, user_id, active_persona, + None, max_iterations, entry, fitter, @@ -1990,6 +1992,7 @@ impl InsightChatService { normalized, user_id, active_persona, + None, ) .await; let (result_preview, result_truncated) = truncate_tool_result(&result); @@ -2624,6 +2627,7 @@ pub async fn run_streaming_agentic_loop_with_entry( normalized: &str, user_id: i32, active_persona: &str, + thread_contact: Option, max_iterations: usize, entry: &Arc, fitter: &mut ContextFitter, @@ -2720,6 +2724,7 @@ pub async fn run_streaming_agentic_loop_with_entry( normalized, user_id, active_persona, + thread_contact, ) .await } else { diff --git a/src/ai/insight_generator.rs b/src/ai/insight_generator.rs index 11e9644..1792a03 100644 --- a/src/ai/insight_generator.rs +++ b/src/ai/insight_generator.rs @@ -1891,6 +1891,7 @@ Return ONLY the summary, nothing else."#, file_path: &str, user_id: i32, persona_id: &str, + thread_contact: Option, ) -> String { let model = backend.model(); let backend_label = backend.kind.as_str(); @@ -1940,9 +1941,12 @@ Return ONLY the summary, nothing else."#, "date_info" => { crate::ai::date_info::date_info(arguments, Utc::now(), self.display_zone()) } - "get_thread_messages" => self.tool_get_thread_messages(arguments).await, + "get_thread_messages" => { + self.tool_get_thread_messages(arguments, thread_contact) + .await + } "look_at_message_image" => { - self.tool_look_at_message_image(arguments, backend.local()) + self.tool_look_at_message_image(arguments, thread_contact, backend.local()) .await } "get_photos_near_time" => self.tool_get_photos_near_time(arguments).await, @@ -2411,9 +2415,14 @@ Return ONLY the summary, nothing else."#, /// Tool: get_thread_messages — page the anchored SMS thread from a /// message id. Lines are rendered exactly like the anchor snapshot so /// the model can keep paging from any id it sees. - async fn tool_get_thread_messages(&self, args: &serde_json::Value) -> String { - let Some(contact_id) = args.get("contact_id").and_then(|v| v.as_i64()) else { - return "Error: missing required parameter 'contact_id'".to_string(); + async fn tool_get_thread_messages( + &self, + args: &serde_json::Value, + thread_contact: Option, + ) -> String { + let contact_id = match Self::bound_thread(args, thread_contact) { + Ok(c) => c, + Err(e) => return e, }; let Some(message_id) = args.get("message_id").and_then(|v| v.as_i64()) else { return "Error: missing required parameter 'message_id'".to_string(); @@ -2461,10 +2470,12 @@ Return ONLY the summary, nothing else."#, async fn tool_look_at_message_image( &self, args: &serde_json::Value, + thread_contact: Option, local: &dyn LlmClient, ) -> String { - let Some(contact_id) = args.get("contact_id").and_then(|v| v.as_i64()) else { - return "Error: missing required parameter 'contact_id'".to_string(); + let contact_id = match Self::bound_thread(args, thread_contact) { + Ok(c) => c, + Err(e) => return e, }; let Some(message_id) = args.get("message_id").and_then(|v| v.as_i64()) else { return "Error: missing required parameter 'message_id'".to_string(); @@ -2476,11 +2487,15 @@ Return ONLY the summary, nothing else."#, let bytes = match self .sms_client - .fetch_mms_media(contact_id, message_id, media_index) + .fetch_mms_image(contact_id, message_id, media_index) .await { Ok(b) => b, - Err(e) => return format!("Error fetching attachment: {}", e), + Err(e) => { + return format!( + "Error: couldn't look at that attachment ({e}); only photos can be viewed." + ); + } }; let image_base64 = match Self::encode_image_bytes_for_vision(&bytes) { Ok(b64) => b64, @@ -2501,7 +2516,12 @@ Return ONLY the summary, nothing else."#, async fn tool_get_photos_near_time(&self, args: &serde_json::Value) -> String { let zone = self.display_zone(); let server_zone = crate::camera_tz::server_zone(); - let Some(center) = Self::parse_moment(args, zone) else { + let center_arg = args + .get("timestamp") + .filter(|v| !v.is_null()) + .or_else(|| args.get("datetime")); + let Some(center) = center_arg.and_then(|v| crate::ai::date_info::parse_center(v, zone)) + else { return "Error: pass 'timestamp' (unix seconds) or 'datetime' (YYYY-MM-DD HH:MM)" .to_string(); }; @@ -2553,11 +2573,22 @@ Return ONLY the summary, nothing else."#, let when = crate::date_view::row_display_wall(&row, zone, server_zone) .map(|w| w.format("%Y-%m-%d %H:%M").to_string()) .unwrap_or_else(|| "unknown time".to_string()); + // Tags follow the bytes: union over every path holding the same + // content, so a copy in another library shows its tags too. + let sibling_paths = { + let mut dao = self.exif_dao.lock().expect("Unable to lock ExifDao"); + dao.get_rel_paths_sharing_content(row.library_id, &row.file_path) + .unwrap_or_else(|_| vec![row.file_path.clone()]) + }; let tags: Vec = { let mut dao = self.tag_dao.lock().expect("Unable to lock TagDao"); - dao.get_tags_for_path(&row.file_path) + let mut names: Vec = dao + .get_tags_for_paths(&sibling_paths) .map(|t| t.into_iter().map(|t| t.name).collect()) - .unwrap_or_default() + .unwrap_or_default(); + names.sort(); + names.dedup(); + names }; let people: Vec = match &row.content_hash { Some(hash) => { @@ -2602,6 +2633,14 @@ Return ONLY the summary, nothing else."#, if crate::file_types::is_video_file(std::path::Path::new(&file_path)) { return "Error: that's a video — only photos can be looked at.".to_string(); } + // Only photos the library has indexed: excluded folders (EXCLUDED_DIRS, + // e.g. `private`) are never indexed, so a guessed path into one is + // refused rather than read off disk. + if self.fetch_exif(&file_path).is_none() { + return "Error: that isn't a photo in the library — use a file_path from \ + get_photos_near_time." + .to_string(); + } let image_base64 = match self.load_image_as_base64(&file_path) { Ok(b) => b, Err(e) => return format!("Error loading photo: {}", e), @@ -2612,6 +2651,26 @@ Return ONLY the summary, nothing else."#, } } + /// The thread a thread tool may read: the one the chat was started from, + /// fixed server-side. A `contact_id` the model passes is only checked + /// against it — never trusted — so a chat (practice mode especially) can + /// never be talked into reading another conversation. + fn bound_thread(args: &serde_json::Value, thread_contact: Option) -> Result { + let Some(bound) = thread_contact else { + return Err( + "Error: thread tools are only available in a chat started from a \ + text thread." + .to_string(), + ); + }; + match args.get("contact_id").and_then(|v| v.as_i64()) { + Some(asked) if asked != bound => { + Err("Error: only this chat's own text thread can be read here.".to_string()) + } + _ => Ok(bound), + } + } + /// A model-supplied path is joined onto each library root, so refuse /// anything that could climb out of it. fn photo_path_is_safe(path: &str) -> bool { @@ -2622,28 +2681,6 @@ Return ONLY the summary, nothing else."#, .all(|c| matches!(c, Component::Normal(_) | Component::CurDir)) } - /// A tool's centre moment: `timestamp` (unix seconds) wins, else - /// `datetime` read as a wall clock in the owner's display zone. A bare - /// date centres on midday. - fn parse_moment(args: &serde_json::Value, zone: chrono_tz::Tz) -> Option { - if let Some(ts) = args.get("timestamp").and_then(|v| v.as_i64()) { - return Some(ts); - } - let raw = args.get("datetime").and_then(|v| v.as_str())?.trim(); - let naive = chrono::NaiveDateTime::parse_from_str(raw, "%Y-%m-%d %H:%M") - .or_else(|_| chrono::NaiveDateTime::parse_from_str(raw, "%Y-%m-%dT%H:%M")) - .or_else(|_| chrono::NaiveDateTime::parse_from_str(raw, "%Y-%m-%dT%H:%M:%S")) - .ok() - .or_else(|| { - NaiveDate::parse_from_str(raw, "%Y-%m-%d") - .ok() - .and_then(|d| d.and_hms_opt(12, 0, 0)) - })?; - zone.from_local_datetime(&naive) - .earliest() - .map(|dt| dt.timestamp()) - } - /// Decode arbitrary image bytes (an MMS attachment), shrink to the /// vision models' 1024px working size and re-encode as base64 JPEG — /// the same shape `load_image_as_base64` produces for library files. @@ -4062,12 +4099,11 @@ Return ONLY the summary, nothing else."#, thread (the #numbers in the transcript) — `before` for older messages, `after` for what \ came next, `around` for both sides. Lines carry their own #id, so call again from the \ oldest/newest id to keep paging. \ - Example: `{contact_id: 42, message_id: 812, direction: \"before\", limit: 30}`.", + Example: `{message_id: 812, direction: \"before\", limit: 30}`.", serde_json::json!({ "type": "object", - "required": ["contact_id", "message_id"], + "required": ["message_id"], "properties": { - "contact_id": { "type": "integer", "description": "The thread's contact_id (given in the system prompt)." }, "message_id": { "type": "integer", "description": "Message id to page from." }, "direction": { "type": "string", "enum": ["before", "after", "around"], "description": "Which side of message_id to read (default before)." }, "limit": { "type": "integer", "description": "Max messages (default 30, max 80)." } @@ -4080,12 +4116,11 @@ Return ONLY the summary, nothing else."#, "Look at a photo that was sent in the SMS thread, using the local vision model. Returns a \ visual description. Attachments appear in the transcript as `image #N` on a message line — \ pass that message's id and N as media_index. Videos and audio can't be viewed. \ - Example: `{contact_id: 42, message_id: 815, media_index: 0}`.", + Example: `{message_id: 815, media_index: 0}`.", serde_json::json!({ "type": "object", - "required": ["contact_id", "message_id"], + "required": ["message_id"], "properties": { - "contact_id": { "type": "integer", "description": "The thread's contact_id." }, "message_id": { "type": "integer", "description": "Id of the message carrying the photo." }, "media_index": { "type": "integer", "description": "Which attachment on that message (default 0)." } } @@ -4843,6 +4878,7 @@ Return ONLY the summary, nothing else."#, &file_path, user_id, &persona_id, + None, ) .instrument(loop_span.clone()) .await; @@ -4895,6 +4931,7 @@ Return ONLY the summary, nothing else."#, &file_path, user_id, &persona_id, + None, ) .instrument(loop_span.clone()) .await; @@ -5083,6 +5120,7 @@ Return ONLY the summary, nothing else."#, "", 0, "", + None, ) .await; messages.push(ChatMessage::tool_result(result)); @@ -5124,6 +5162,7 @@ Return ONLY the summary, nothing else."#, "", 0, "", + None, ) .await; messages.push(ChatMessage::tool_result(result)); @@ -5294,6 +5333,21 @@ mod tests { assert!(names.contains(&"look_at_photo")); } + #[test] + fn thread_tools_only_read_the_chats_own_thread() { + use serde_json::json; + // The bound thread is used; the model needn't (and can't) choose. + assert_eq!(InsightGenerator::bound_thread(&json!({}), Some(42)), Ok(42)); + assert_eq!( + InsightGenerator::bound_thread(&json!({"contact_id": 42}), Some(42)), + Ok(42) + ); + // Another thread is refused, even if the model names it. + assert!(InsightGenerator::bound_thread(&json!({"contact_id": 7}), Some(42)).is_err()); + // No bound thread (not a thread chat): nothing to read. + assert!(InsightGenerator::bound_thread(&json!({"contact_id": 42}), None).is_err()); + } + #[test] fn photo_path_is_safe_rejects_escapes() { assert!(InsightGenerator::photo_path_is_safe("2019/06/IMG_1.jpg")); @@ -5303,32 +5357,6 @@ mod tests { assert!(!InsightGenerator::photo_path_is_safe("")); } - #[test] - fn parse_moment_accepts_unix_and_local_wall_clock() { - let tz = chrono_tz::Tz::America__New_York; - assert_eq!( - InsightGenerator::parse_moment(&serde_json::json!({"timestamp": 1_700_000_000}), tz), - Some(1_700_000_000) - ); - // 2023-11-14 17:13 in New York (EST) = 22:13 UTC. - assert_eq!( - InsightGenerator::parse_moment( - &serde_json::json!({"datetime": "2023-11-14 17:13"}), - tz - ), - Some(1_699_999_980) - ); - // A bare date means midday, the least-wrong centre for a whole day. - assert_eq!( - InsightGenerator::parse_moment(&serde_json::json!({"datetime": "2023-11-14"}), tz), - Some(1_699_981_200) - ); - assert_eq!( - InsightGenerator::parse_moment(&serde_json::json!({}), tz), - None - ); - } - fn place(name: &str, description: &str) -> ApolloPlace { ApolloPlace { id: 1, diff --git a/src/ai/persona_chat.rs b/src/ai/persona_chat.rs index e8910ea..6b3bb8c 100644 --- a/src/ai/persona_chat.rs +++ b/src/ai/persona_chat.rs @@ -122,6 +122,40 @@ pub fn seed_messages_for_persona(system_prompt: &str) -> Vec { vec![ChatMessage::system(system_prompt.to_string())] } +/// The persona id a practice conversation is stored under. Practice mode +/// plays the SMS contact, not a persona, so the conversation belongs to no +/// persona row: deleting or editing personas can't hide or break it. +pub const PRACTICE_PERSONA_ID: &str = ""; + +/// Name shown for a practice conversation where a persona name would go. +pub const PRACTICE_PERSONA_NAME: &str = "Practice"; + +/// Whether a stored conversation is a practice one (plays the contact). +fn is_practice(row: &crate::database::persona_dao::PersonaChatRow) -> bool { + row.anchor + .as_ref() + .is_some_and(|a| a.mode == crate::database::AnchorMode::AsContact) +} + +/// The stand-in persona a practice turn runs with. Its prompt is never used +/// (`anchored_system_prompt` ignores the persona in practice mode) and it +/// can't correct facts — practice is offered no knowledge tools at all. +fn practice_persona(user_id: i32) -> crate::database::models::Persona { + crate::database::models::Persona { + id: 0, + user_id, + persona_id: PRACTICE_PERSONA_ID.to_string(), + name: PRACTICE_PERSONA_NAME.to_string(), + system_prompt: String::new(), + is_built_in: false, + include_all_memories: false, + created_at: 0, + updated_at: 0, + reviewed_only_facts: false, + allow_agent_corrections: false, + } +} + /// Parse a persisted transcript into the conversation tree. /// /// Two on-disk formats are accepted, mirroring the file chat's reader: @@ -602,8 +636,13 @@ impl PersonaChatSession { message_id: i64, mode: crate::database::AnchorMode, ) -> Result { - let persona_id = persona_id.trim(); - { + // Practice plays the contact; any persona the client sent is ignored. + let persona_id = if mode == crate::database::AnchorMode::AsContact { + PRACTICE_PERSONA_ID + } else { + persona_id.trim() + }; + if mode != crate::database::AnchorMode::AsContact { let mut dao = self.persona_dao.lock().expect("persona_dao poisoned"); let persona = dao .get_persona(user_id, persona_id) @@ -740,10 +779,13 @@ impl PersonaChatSession { .get_persona_chat(user_id, &conversation_id) .map_err(|e| PersonaChatError::Db(anyhow!("{e:?}")))? .ok_or_else(|| PersonaChatError::UnknownConversation(conversation_id.clone()))?; - let persona = dao - .get_persona(user_id, &row.persona_id) - .map_err(|e| PersonaChatError::Db(anyhow!("{e:?}")))? - .ok_or_else(|| PersonaChatError::UnknownPersona(row.persona_id.clone()))?; + let persona = if is_practice(&row) { + practice_persona(user_id) + } else { + dao.get_persona(user_id, &row.persona_id) + .map_err(|e| PersonaChatError::Db(anyhow!("{e:?}")))? + .ok_or_else(|| PersonaChatError::UnknownPersona(row.persona_id.clone()))? + }; (persona, row.persona_id) }; @@ -1004,6 +1046,8 @@ impl PersonaChatSession { "", user_id, &persona.persona_id, + // Thread tools only ever read this chat's own thread. + row.anchor.as_ref().map(|a| a.contact_id), max_iterations, &entry, &mut fitter, @@ -1119,9 +1163,11 @@ impl PersonaChatSession { ) -> Result> { let (rows, personas) = { let mut dao = self.persona_dao.lock().expect("persona_dao poisoned"); - let rows = dao - .list_persona_chats(user_id) - .map_err(|e| anyhow!("failed to list persona chats: {e:?}"))?; + let rows = match contact_id { + Some(cid) => dao.list_persona_chats_for_contact(user_id, cid), + None => dao.list_persona_chats(user_id), + } + .map_err(|e| anyhow!("failed to list persona chats: {e:?}"))?; let personas = dao .list_personas(user_id) .map_err(|e| anyhow!("failed to list personas: {e:?}"))?; @@ -1134,13 +1180,13 @@ impl PersonaChatSession { let mut out = Vec::with_capacity(rows.len()); for entry in rows { - if let Some(cid) = contact_id - && entry.anchor.as_ref().map(|a| a.contact_id) != Some(cid) - { - continue; - } - let Some(persona_name) = names.get(&entry.persona_id).cloned() else { - continue; + let persona_name = if is_practice(&entry) { + PRACTICE_PERSONA_NAME.to_string() + } else { + match names.get(&entry.persona_id) { + Some(name) => name.clone(), + None => continue, + } }; // A single unreadable transcript shouldn't take the whole list // down with it — skip the row and keep going. @@ -1372,6 +1418,9 @@ pub struct PersonaChatHistoryQuery { /// Body for POST /persona_chat/conversations — start a new conversation. #[derive(Debug, Deserialize)] pub struct PersonaChatCreateRequest { + /// Required except for a practice (`as_contact`) anchor, which plays + /// the contact rather than a persona. + #[serde(default)] pub persona_id: String, /// Anchor the conversation on a message in an SMS thread ("Continue /// with persona"). Omitted for an open chat. @@ -2038,6 +2087,24 @@ mod tests { assert!(tracker.claim("conv-a", "t7").is_ok()); } + #[test] + fn practice_create_request_needs_no_persona() { + let req: PersonaChatCreateRequest = serde_json::from_str( + r#"{"anchor":{"contact_id":42,"message_id":812,"mode":"as_contact"}}"#, + ) + .unwrap(); + assert_eq!(req.persona_id, ""); + } + + #[test] + fn practice_persona_is_inert() { + let p = practice_persona(7); + assert_eq!(p.persona_id, PRACTICE_PERSONA_ID); + assert_eq!(p.user_id, 7); + assert!(p.system_prompt.is_empty()); + assert!(!p.allow_agent_corrections); + } + #[test] fn create_request_anchor_mode_defaults_to_about() { use crate::database::AnchorMode; diff --git a/src/ai/sms_anchor.rs b/src/ai/sms_anchor.rs index 65018f3..8f69032 100644 --- a/src/ai/sms_anchor.rs +++ b/src/ai/sms_anchor.rs @@ -139,7 +139,6 @@ struct PromptParts { contact: String, when: String, thread: String, - cid: i64, mid: i64, } @@ -165,7 +164,6 @@ fn prompt_parts(anchor: &SmsAnchor, tz: Tz) -> PromptParts { contact, when, thread, - cid: anchor.contact_id, mid: anchor.message_id, } } @@ -196,7 +194,6 @@ fn about_prompt(persona_prompt: &str, parts: &PromptParts, user_name: &str) -> S contact, when, thread, - cid, mid, } = parts; format!( @@ -227,11 +224,11 @@ the lookups. Look at one or two photos that matter, not every one you find. - You can read and look, but you can't send photos or texts to anyone — \ don't offer to. -Tools for this thread (contact_id {cid}; #numbers below are message ids): -- get_thread_messages {{contact_id: {cid}, message_id: {mid}, direction: \"before\"}} \ -— earlier messages; \"after\" for what came next. -- look_at_message_image {{contact_id: {cid}, message_id, media_index}} — see a \ -photo sent in the thread (attachments are listed as `image #N`). +Tools for this thread (#numbers below are message ids): +- get_thread_messages {{message_id: {mid}, direction: \"before\"}} — earlier \ +messages; \"after\" for what came next. +- look_at_message_image {{message_id, media_index}} — see a photo sent in the \ +thread (attachments are listed as `image #N`). - get_photos_near_time {{timestamp}} — photos from {user_name}'s library taken \ around a moment, with their tags and the people in them. - look_at_photo {{file_path}} — see one of those library photos. @@ -247,23 +244,40 @@ fn practice_prompt(parts: &PromptParts, user_name: &str) -> String { contact, when, thread, - cid, mid, } = parts; format!( - "You are playing {contact} in a text conversation with {user_name}, so {user_name} can practise picking the conversation back up. This is a private rehearsal for {user_name}; the real {contact} is not part of it. + "You are playing {contact} in a text conversation with {user_name}, so \ +{user_name} can practise picking the conversation back up. This is a private \ +rehearsal for {user_name}; the real {contact} is not part of it. -Below is the real thread up to the message marked ▶ (sent {when}). Carry on from there as {contact}: {user_name}'s messages to you are their next texts, and every reply you write is {contact}'s next text. +Below is the real thread up to the message marked ▶ (sent {when}). Carry on \ +from there as {contact}: {user_name}'s messages to you are their next texts, \ +and every reply you write is {contact}'s next text. How to play it: -- Sound like {contact} does in the thread — their message length, slang, emoji, punctuation, humour, how fast they get to the point. Mirror them, not a generic texter. -- Write only the text itself. No narration, no stage directions, no 'As {contact}, I\'d say…'. Separate several short texts with a blank line. -- Stay true to what {contact} knows and feels in the thread. Don't invent big facts about their life — jobs, relationships, events — that the thread doesn't support; if asked about something they couldn't know, answer vaguely or deflect the way they plausibly would. -- React like a real person: it's fine to be busy, short, unconvinced, or to ask something back. Don't be more agreeable than {contact} actually is in the thread. +- Sound like {contact} does in the thread — their message length, slang, \ +emoji, punctuation, humour, how fast they get to the point. Mirror them, not \ +a generic texter. +- Write only the text itself. No narration, no stage directions, no 'As \ +{contact}, I\'d say…'. Separate several short texts with a blank line. +- Stay true to what {contact} knows and feels in the thread. Don't invent \ +big facts about their life — jobs, relationships, events — that the thread \ +doesn't support; if asked about something they couldn't know, answer vaguely \ +or deflect the way they plausibly would. +- React like a real person: it's fine to be busy, short, unconvinced, or to \ +ask something back. Don't be more agreeable than {contact} actually is in \ +the thread. - It is shortly after the ▶ message unless {user_name} says time has passed. -- If {user_name} steps out of the rehearsal ('pause', 'out of character', 'how did that sound?'), drop the role and answer briefly as a coach: honest, specific feedback on how their texts are landing. Go back to being {contact} when they carry on. +- If {user_name} steps out of the rehearsal ('pause', 'out of character', \ +'how did that sound?'), drop the role and answer briefly as a coach: honest, \ +specific feedback on how their texts are landing. Go back to being {contact} \ +when they carry on. -If you need to remember an earlier part of the thread, use get_thread_messages {{contact_id: {cid}, message_id: {mid}, direction: \"before\"}}; to see a photo sent in it (`image #N` on a line), use look_at_message_image {{contact_id: {cid}, message_id, media_index}}. Never mention either. +If you need to remember an earlier part of the thread, use \ +get_thread_messages {{message_id: {mid}, direction: \"before\"}}; to see a \ +photo sent in it (`image #N` on a line), use look_at_message_image \ +{{message_id, media_index}}. Never mention either. Thread between {user_name} and {contact}: {thread}" @@ -368,8 +382,10 @@ mod tests { }; let prompt = anchored_system_prompt("You are Journal.", &anchor, "Cam", Tz::UTC); assert!(prompt.starts_with("You are Journal.")); - assert!(prompt.contains("contact_id 42")); assert!(prompt.contains("message_id: 812")); + // The thread is bound server-side; the prompt doesn't hand the + // model a contact id to choose. + assert!(!prompt.contains("contact_id")); assert!(prompt.contains("You are not Sam")); assert!(prompt.ends_with("Sam: see you there")); } @@ -405,7 +421,8 @@ mod tests { !prompt.contains("You are Journal."), "a persona voice would bleed into the contact's" ); - assert!(prompt.contains("contact_id: 42")); + assert!(prompt.contains("message_id: 812")); + assert!(!prompt.contains("contact_id")); assert!(prompt.contains("out of character")); assert!(prompt.ends_with("Sam: see you there")); } diff --git a/src/ai/sms_client.rs b/src/ai/sms_client.rs index 48b66d4..505ac6f 100644 --- a/src/ai/sms_client.rs +++ b/src/ai/sms_client.rs @@ -425,10 +425,14 @@ impl SmsApiClient { }) } - /// Download one MMS attachment. `media_index` is the attachment's - /// position among the message's image/video/audio parts — the same - /// index SMS-API's `/media/{message}/{index}/file/` route takes. - pub async fn fetch_mms_media( + /// Download one MMS *image*. `media_index` is the attachment's position + /// among the message's image/video/audio parts — the same index + /// SMS-API's `/media/{message}/{index}/file/` route takes. + /// + /// Checks the response before reading the body, so pointing this at a + /// video or audio attachment costs a header round-trip, not the whole + /// file in memory; images over [`MMS_IMAGE_MAX_BYTES`] are refused too. + pub async fn fetch_mms_image( &self, contact_id: i64, message_id: i64, @@ -442,14 +446,29 @@ impl SmsApiClient { if let Some(token) = &self.token { request = request.header("Authorization", format!("Bearer {}", token)); } - let response = request.send().await?; + let mut response = request.send().await?; if !response.status().is_success() { return Err(anyhow::anyhow!( "MMS media request failed: {}", response.status() )); } - Ok(response.bytes().await?.to_vec()) + let content_type = response + .headers() + .get(reqwest::header::CONTENT_TYPE) + .and_then(|v| v.to_str().ok()) + .map(str::to_string); + check_mms_image_response(content_type.as_deref(), response.content_length()) + .map_err(|e| anyhow::anyhow!(e))?; + // Content-Length can be absent (chunked); cap while reading too. + let mut bytes = Vec::new(); + while let Some(chunk) = response.chunk().await? { + if bytes.len() + chunk.len() > MMS_IMAGE_MAX_BYTES { + return Err(anyhow::anyhow!("attachment is larger than the image limit")); + } + bytes.extend_from_slice(&chunk); + } + Ok(bytes) } pub async fn summarize_context( @@ -504,6 +523,29 @@ pub struct SmsMessage { pub is_sent: bool, } +/// Largest MMS image `fetch_mms_image` will download. Phone photos sent by +/// MMS are a few MB; anything past this is not worth decoding for a +/// 1024px vision description. +pub const MMS_IMAGE_MAX_BYTES: usize = 20 * 1024 * 1024; + +/// Decide from the response headers whether an MMS attachment is an image +/// worth downloading. A missing content type is allowed through (the +/// decoder decides); a declared non-image or an oversized length is not. +fn check_mms_image_response( + content_type: Option<&str>, + content_length: Option, +) -> Result<(), String> { + if let Some(ct) = content_type + && !ct.trim().to_ascii_lowercase().starts_with("image/") + { + return Err(format!("attachment is {ct}, not an image")); + } + if content_length.is_some_and(|n| n > MMS_IMAGE_MAX_BYTES as u64) { + return Err("attachment is larger than the image limit".to_string()); + } + Ok(()) +} + /// A thread participant as SMS-API's conversation endpoint describes it. #[derive(Debug, Clone, Deserialize)] pub struct ThreadContact { @@ -747,6 +789,23 @@ mod tests { ); } + #[test] + fn mms_image_check_refuses_non_images_and_huge_files_before_download() { + assert!(check_mms_image_response(Some("image/jpeg"), Some(2_000_000)).is_ok()); + assert!(check_mms_image_response(Some("IMAGE/PNG"), None).is_ok()); + assert!(check_mms_image_response(None, None).is_ok()); + assert!( + check_mms_image_response(Some("video/mp4"), Some(1)) + .unwrap_err() + .contains("video/mp4") + ); + assert!(check_mms_image_response(Some("audio/amr"), None).is_err()); + assert!( + check_mms_image_response(Some("image/jpeg"), Some(MMS_IMAGE_MAX_BYTES as u64 + 1)) + .is_err() + ); + } + #[test] fn thread_contact_falls_back_to_the_address() { let c = ThreadContact { diff --git a/src/database/persona_dao.rs b/src/database/persona_dao.rs index 7138f03..f3e7231 100644 --- a/src/database/persona_dao.rs +++ b/src/database/persona_dao.rs @@ -232,6 +232,15 @@ pub trait PersonaDao: Sync + Send { /// derived from the tree rather than denormalized into its own column. fn list_persona_chats(&mut self, user_id: i32) -> Result, DbError>; + /// Conversations anchored on one SMS thread, newest first. Backs the + /// Messages screen's per-thread list; served by + /// `idx_persona_chat_anchor_contact`. + fn list_persona_chats_for_contact( + &mut self, + user_id: i32, + contact_id: i64, + ) -> Result, DbError>; + /// Start a new conversation with a persona, returning its id. Several /// conversations with the same persona are expected, so this never /// reuses an existing row. @@ -524,6 +533,26 @@ impl PersonaDao for SqlitePersonaDao { .map_err(|e| DbError::log(DbErrorKind::QueryError, e)) } + fn list_persona_chats_for_contact( + &mut self, + uid: i32, + cid: i64, + ) -> Result, DbError> { + trace_db_call("query", "list_persona_chats_for_contact", |_span| { + use schema::persona_chat_conversations::dsl::*; + let mut conn = self.connection.lock().expect("PersonaDao lock"); + persona_chat_conversations + .filter(user_id.eq(uid)) + .filter(anchor_contact_id.eq(cid)) + .order(updated_at.desc()) + .select(persona_chat_select!()) + .load::(conn.deref_mut()) + .map(|rows| rows.into_iter().map(persona_chat_row).collect()) + .map_err(|e| anyhow::anyhow!("Query error: {}", e)) + }) + .map_err(|e| DbError::log(DbErrorKind::QueryError, e)) + } + fn create_persona_chat( &mut self, uid: i32, @@ -932,6 +961,37 @@ mod tests { ); } + #[test] + fn persona_chat_list_for_contact_returns_only_that_thread() { + let (mut dao, uid) = dao_with_user("p11"); + let anchor = |contact_id| SmsAnchor { + contact_id, + contact_name: "x".to_string(), + message_id: 1, + timestamp: 0, + context: String::new(), + mode: AnchorMode::About, + }; + let older = dao + .create_anchored_persona_chat(uid, "journal", 100, &anchor(42)) + .unwrap(); + let _other = dao + .create_anchored_persona_chat(uid, "journal", 150, &anchor(7)) + .unwrap(); + let _open = dao.create_persona_chat(uid, "journal", 175).unwrap(); + let newer = dao + .create_anchored_persona_chat(uid, "journal", 200, &anchor(42)) + .unwrap(); + + let ids: Vec = dao + .list_persona_chats_for_contact(uid, 42) + .unwrap() + .into_iter() + .map(|r| r.conversation_id) + .collect(); + assert_eq!(ids, vec![newer, older]); + } + #[test] fn anchor_mode_reads_null_and_unknown_as_about() { assert_eq!(AnchorMode::from_column(None), AnchorMode::About);