diff --git a/crates/ironclaw_engine/src/runtime/conversation.rs b/crates/ironclaw_engine/src/runtime/conversation.rs index 3f31ae12fd6..0cbbe9df90b 100644 --- a/crates/ironclaw_engine/src/runtime/conversation.rs +++ b/crates/ironclaw_engine/src/runtime/conversation.rs @@ -218,6 +218,7 @@ impl ConversationManager { user_id: &str, thread_config: ThreadConfig, user_timezone: Option<&str>, + raw_content_for_title: Option<&str>, extra_initial_metadata: Option>, ) -> Result { let conv_arc = self.get_conversation_lock(conversation_id).await?; @@ -351,7 +352,10 @@ impl ConversationManager { // Spawn new foreground thread with conversation history. // `goal` holds the full message (the orchestrator feeds it as // the initial user turn); `title` is the short sidebar label. - let title = crate::types::thread::Thread::derive_title_from_message(content); + // For attachment-augmented turns, derive that title from the + // raw user text so engine thread surfaces do not expose the + // synthesized `` block or extracted attachment text. + let title = thread_title_from_message_sources(content, raw_content_for_title); self.thread_manager .spawn_thread_with_history( content, // use message as goal @@ -372,7 +376,16 @@ impl ConversationManager { // The user entry is added here — after the thread operation succeeded — to // prevent orphaned entries if inject_message/resume_thread/spawn_thread_with_history // returned an error above. - conv.add_entry(ConversationEntry::user(content)); + // Record the user entry with a separate `title_source` so downstream + // consumers deriving a conversation title use the raw user text + // instead of the attachment-augmented payload. `content` is still + // the LLM-facing string (augmented with `` / extracted + // OCR); only the title-derivation surface changes. + let entry = match raw_content_for_title { + Some(raw) if raw != content => ConversationEntry::user_with_title_source(content, raw), + _ => ConversationEntry::user(content), + }; + conv.add_entry(entry); match active_foreground { Some(ActiveForeground::Running(_)) => { // No additional in-memory mutation needed beyond the user entry above. @@ -611,6 +624,18 @@ fn build_history_from_entries( .collect() } +fn thread_title_from_message_sources( + content: &str, + raw_content_for_title: Option<&str>, +) -> Option { + let Some(raw) = raw_content_for_title else { + return crate::types::thread::Thread::derive_title_from_message(content); + }; + + crate::types::thread::Thread::derive_title_from_message(raw) + .or_else(|| Some("Untitled chat".to_string())) +} + #[cfg(test)] mod tests { use super::*; @@ -905,6 +930,7 @@ mod tests { ThreadConfig::default(), None, None, + None, ) .await .unwrap(); @@ -974,6 +1000,7 @@ mod tests { ThreadConfig::default(), None, None, + None, ) .await .unwrap(); @@ -1074,6 +1101,7 @@ mod tests { ThreadConfig::default(), None, None, + None, ) .await .unwrap(); @@ -1124,6 +1152,7 @@ mod tests { ThreadConfig::default(), None, None, + None, ) .await }); @@ -1136,6 +1165,7 @@ mod tests { ThreadConfig::default(), None, None, + None, ) .await }); @@ -1213,6 +1243,7 @@ mod tests { ThreadConfig::default(), None, None, + None, ) .await .unwrap(); @@ -1226,6 +1257,162 @@ mod tests { ); } + /// Engine-v2 `handle_user_message` must preserve the raw user text + /// on the `ConversationEntry` as `title_source` metadata when the + /// caller supplies attachment-augmented content. This closes the + /// Issue 2 regression where the sidebar / downstream title derivation + /// would see the synthesized `` block rather than the + /// user-typed text. + #[tokio::test] + async fn handle_user_message_records_raw_title_source() { + let (_tm, cm) = make_conv_manager(); + let conv_id = cm + .get_or_create_conversation("web", "user-raw") + .await + .unwrap(); + let project = ProjectId::new(); + + let raw = "Summarise the attached doc"; + let augmented = "Summarise the attached doc\n\nQ3 rev $4.2M\n"; + + let _tid = cm + .handle_user_message( + conv_id, + augmented, + project, + "user-raw", + ThreadConfig::default(), + None, + Some(raw), + None, + ) + .await + .unwrap(); + + let conv = cm.get_conversation(conv_id).await.unwrap(); + let user_entry = conv + .entries + .iter() + .find(|e| matches!(e.sender, crate::types::conversation::EntrySender::User)) + .expect("user entry present"); + // LLM-facing content stays augmented. + assert_eq!(user_entry.content, augmented); + // Title derivation surface is the raw user text. + let title_source = user_entry + .metadata + .get("title_source") + .and_then(|v| v.as_str()); + assert_eq!( + title_source, + Some(raw), + "title_source must be the raw user text, not the augmented payload" + ); + } + + /// The engine thread's user-visible title must also be derived from raw + /// user text, while `goal` and the LLM-facing first message keep the + /// augmented payload. Otherwise thread-list/detail surfaces that expose + /// `Thread.title` can still leak `` blocks even though the + /// conversation-entry title_source is correct. + #[tokio::test] + async fn handle_user_message_uses_raw_title_source_as_thread_title() { + let store = Arc::new(MockStore::new()); + let tm = Arc::new(ThreadManager::new( + Arc::new(MockLlm(Mutex::new(vec![LlmOutput { + response: LlmResponse::Text("Hello!".into()), + usage: TokenUsage::default(), + }]))), + Arc::new(MockEffects), + store.clone(), + Arc::new(CapabilityRegistry::new()), + Arc::new(LeaseManager::new()), + Arc::new(PolicyEngine::new()), + )); + let cm = ConversationManager::new(Arc::clone(&tm), store.clone()); + let conv_id = cm + .get_or_create_conversation("web", "user-goal") + .await + .unwrap(); + let project = ProjectId::new(); + + let raw = "Summarise the attached doc"; + let augmented = "Summarise the attached doc\n\nQ3 rev $4.2M\n"; + + let tid = cm + .handle_user_message( + conv_id, + augmented, + project, + "user-goal", + ThreadConfig::default(), + None, + Some(raw), + None, + ) + .await + .unwrap(); + + let thread = store.load_thread(tid).await.unwrap().expect("thread"); + assert_eq!( + thread.goal, augmented, + "thread goal should preserve the execution prompt" + ); + assert_eq!( + thread.title.as_deref(), + Some(raw), + "thread title should be display-safe raw text" + ); + let first_user_message = thread + .messages + .iter() + .find(|m| m.role == MessageRole::User) + .expect("first user message"); + assert_eq!( + first_user_message.content, augmented, + "LLM-facing message must keep attachment context" + ); + } + + /// Plain-text turns (no augmentation) must NOT stamp a redundant + /// `title_source` — `content` already is the title source. + #[tokio::test] + async fn handle_user_message_plain_text_has_no_title_source_metadata() { + let (_tm, cm) = make_conv_manager(); + let conv_id = cm + .get_or_create_conversation("web", "user-plain") + .await + .unwrap(); + let project = ProjectId::new(); + + let plain = "what's the weather"; + let _tid = cm + .handle_user_message( + conv_id, + plain, + project, + "user-plain", + ThreadConfig::default(), + None, + Some(plain), + None, + ) + .await + .unwrap(); + + let conv = cm.get_conversation(conv_id).await.unwrap(); + let user_entry = conv + .entries + .iter() + .find(|e| matches!(e.sender, crate::types::conversation::EntrySender::User)) + .expect("user entry"); + assert_eq!(user_entry.content, plain); + assert!( + user_entry.metadata.is_null(), + "plain-text turn must leave metadata unset, got {:?}", + user_entry.metadata + ); + } + #[tokio::test] async fn record_external_agent_message_rejects_wrong_user() { let (_, cm) = make_conv_manager(); diff --git a/crates/ironclaw_engine/src/types/conversation.rs b/crates/ironclaw_engine/src/types/conversation.rs index 7a16ecee170..1b1a2c42fb8 100644 --- a/crates/ironclaw_engine/src/types/conversation.rs +++ b/crates/ironclaw_engine/src/types/conversation.rs @@ -86,6 +86,32 @@ impl ConversationEntry { } } + /// Create a user entry with a separate `title_source` — the raw + /// user-typed text before any attachment augmentation. `content` + /// remains the LLM-facing payload (augmented with attachment blocks / + /// extracted text); `title_source` is recorded in `metadata` so that + /// downstream consumers deriving a sidebar title use the raw text + /// rather than the synthesized attachment block. + pub fn user_with_title_source( + content: impl Into, + title_source: impl Into, + ) -> Self { + let title_source = title_source.into(); + let metadata = if title_source.is_empty() { + serde_json::Value::Null + } else { + serde_json::json!({ "title_source": title_source }) + }; + Self { + id: EntryId::new(), + sender: EntrySender::User, + content: content.into(), + origin_thread_id: None, + timestamp: Utc::now(), + metadata, + } + } + /// Create an agent entry from a thread. pub fn agent(thread_id: ThreadId, content: impl Into) -> Self { Self { diff --git a/crates/ironclaw_gateway/static/i18n/en.js b/crates/ironclaw_gateway/static/i18n/en.js index ab673b8cd6a..db64690d35c 100644 --- a/crates/ironclaw_gateway/static/i18n/en.js +++ b/crates/ironclaw_gateway/static/i18n/en.js @@ -840,6 +840,8 @@ I18n.register('en', { // Thread types 'thread.heartbeatAlerts': 'Heartbeat Alerts', 'thread.routine': 'Routine', + 'thread.newChat': 'New chat', + 'thread.untitled': 'Untitled chat', // Extensions (dynamic) 'extensions.openingAuth': 'Opening authentication for {name}', diff --git a/crates/ironclaw_gateway/static/i18n/ko.js b/crates/ironclaw_gateway/static/i18n/ko.js index 8dfc75de447..e88cab27a64 100644 --- a/crates/ironclaw_gateway/static/i18n/ko.js +++ b/crates/ironclaw_gateway/static/i18n/ko.js @@ -813,6 +813,8 @@ I18n.register('ko', { // 스레드 유형 'thread.heartbeatAlerts': '하트비트 알림', 'thread.routine': '루틴', + 'thread.newChat': '새 대화', + 'thread.untitled': '제목 없는 대화', // 확장 (동적) 'extensions.openingAuth': '{name}에 대한 인증을 여는 중', diff --git a/crates/ironclaw_gateway/static/i18n/zh-CN.js b/crates/ironclaw_gateway/static/i18n/zh-CN.js index bd8f386a966..3123d0f8e10 100644 --- a/crates/ironclaw_gateway/static/i18n/zh-CN.js +++ b/crates/ironclaw_gateway/static/i18n/zh-CN.js @@ -839,6 +839,8 @@ I18n.register('zh-CN', { // 线程类型 'thread.heartbeatAlerts': '心跳提醒', 'thread.routine': '定时任务', + 'thread.newChat': '新对话', + 'thread.untitled': '未命名对话', // 扩展(动态) 'extensions.openingAuth': '正在为 {name} 打开认证', diff --git a/crates/ironclaw_gateway/static/js/core/history.js b/crates/ironclaw_gateway/static/js/core/history.js index e6257bea9aa..a77d73a6cef 100644 --- a/crates/ironclaw_gateway/static/js/core/history.js +++ b/crates/ironclaw_gateway/static/js/core/history.js @@ -361,8 +361,8 @@ function threadTitle(thread) { if (thread.thread_type === 'heartbeat') return I18n.t('thread.heartbeatAlerts'); if (thread.thread_type === 'routine') return I18n.t('thread.routine'); if (ch !== 'gateway') return ch.charAt(0).toUpperCase() + ch.slice(1); - if (thread.turn_count === 0) return 'New chat'; - return thread.id.substring(0, 8); + if (thread.turn_count === 0) return I18n.t('thread.newChat'); + return I18n.t('thread.untitled'); } function relativeTime(isoStr) { diff --git a/src/agent/session.rs b/src/agent/session.rs index 3478d9be678..5968f7a8f21 100644 --- a/src/agent/session.rs +++ b/src/agent/session.rs @@ -718,8 +718,16 @@ pub struct Turn { /// Persisted user message ID when this turn has been written to the DB. #[serde(default, skip_serializing_if = "Option::is_none")] pub user_message_id: Option, - /// User input that started this turn. + /// User input that started this turn. On the v1 attachment pipeline + /// this is the *augmented* payload (raw text + synthesized + /// `` block) that is fed to the LLM. pub user_input: String, + /// Raw user-typed text for the turn, before attachment augmentation. + /// Used for sidebar / title derivation so that attachment-only or + /// augmented turns do not surface the synthesized attachment block as + /// the conversation title. `None` for turns that predate this field. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub raw_user_input: Option, /// Agent response (if completed). pub response: Option, /// Tool calls made during this turn. @@ -750,6 +758,7 @@ impl Turn { turn_number, user_message_id: None, user_input: user_input.into(), + raw_user_input: None, response: None, tool_calls: Vec::new(), state: TurnState::Processing, diff --git a/src/agent/thread_ops.rs b/src/agent/thread_ops.rs index 8b6e852d115..bafef9cb3af 100644 --- a/src/agent/thread_ops.rs +++ b/src/agent/thread_ops.rs @@ -654,6 +654,15 @@ impl Agent { .ok_or_else(|| Error::from(crate::error::JobError::NotFound { id: thread_id }))?; let turn = thread.start_turn(effective_content); turn.image_content_parts = image_parts; + // Preserve the raw user text alongside the augmented payload so + // the no-DB / in-memory fallback in the threads endpoint can + // derive the sidebar title from the raw text instead of the + // synthesized `` block. Only set when it differs + // from `user_input` — plain-text turns keep it `None` since + // `user_input` is already correct for title derivation. + if content != effective_content { + turn.raw_user_input = Some(content.to_string()); + } let turn_number = turn.turn_number; let turn_started_at = turn.started_at; (thread.messages(), turn_number, turn_started_at) @@ -672,6 +681,7 @@ impl Agent { &message.user_id, turn_number, effective_content, + content, turn_started_at, ) .await; @@ -1016,6 +1026,19 @@ impl Agent { /// /// This ensures the user message is durable even if the process crashes /// mid-response. Call this right after `thread.start_turn()`. + /// Persist the first-turn user message row and (on success) seed the + /// sidebar title from `title_source`. + /// + /// `user_input` is the attachment-augmented payload that the engine + /// actually processes and gets stored as the conversation row body. + /// `title_source` is the raw user-entered text before attachment + /// augmentation; deriving the sidebar title from the raw text means + /// an image- or attachment-only first turn doesn't claim the title + /// slot with the synthesized `` block or extracted + /// attachment OCR. `set_title_if_missing` already skips empty input, + /// so attachment-only turns naturally defer title-setting to a + /// later turn that carries real text. + #[allow(clippy::too_many_arguments)] pub(super) async fn persist_user_message( &self, thread_id: Uuid, @@ -1023,6 +1046,7 @@ impl Agent { user_id: &str, turn_number: usize, user_input: &str, + title_source: &str, started_at: DateTime, ) -> Option { let store = match self.store() { @@ -1037,7 +1061,7 @@ impl Agent { return None; } - match store + let result = match store .add_conversation_message(thread_id, "user", user_input) .await { @@ -1072,7 +1096,13 @@ impl Agent { .await; None } + }; + + if result.is_some() { + crate::db::set_title_if_missing(store.as_ref(), thread_id, title_source).await; } + + result } /// Persist the assistant response to the DB after the agentic loop completes. @@ -4057,6 +4087,230 @@ mod tests { ); } + /// Caller-level regression for PR #2700 review comments. + /// + /// Drives `persist_user_message` through a real `Agent` + real + /// `LibSqlBackend` store — the actual production persist path — and + /// pins three invariants that unit tests of `set_title_if_missing` + /// alone cannot pin: + /// + /// 1. **Title source.** When the first user turn carries + /// attachments, the sidebar title must be derived from the raw + /// user text (`title_source`), not the augmented `user_input` + /// that includes the synthesized `` block. + /// 2. **Empty-raw skip.** An image- or attachment-only first turn + /// (empty raw text, augmented body) must leave the title unset + /// so a later turn with real text can claim the slot. + /// 3. **Plain text.** A straightforward first user message with no + /// attachments still seeds the title correctly — negative + /// control for the edits above. + /// + /// Driving through the real caller matters because a regression in + /// the caller-side wiring (e.g. swapping `title_source` back to + /// `user_input`, or dropping the `result.is_some()` gate) would + /// leave all predicate-level unit tests green. + #[cfg(feature = "libsql")] + #[tokio::test] + async fn persist_user_message_title_uses_raw_text_not_augmented_payload() { + use crate::db::Database; + use crate::db::libsql::LibSqlBackend; + use chrono::Utc; + use uuid::Uuid; + + // Build a real libSQL-backed store and wire it into a test agent. + let tmp = tempfile::tempdir().expect("tempdir"); + let db_path = tmp.path().join("title_source.db"); + let backend = LibSqlBackend::new_local(&db_path) + .await + .expect("libsql backend"); + backend.run_migrations().await.expect("migrations"); + let store: Arc = Arc::new(backend); + + let (channels, _statuses) = { + let statuses = Arc::new(TokioMutex::new(Vec::new())); + let channels = Arc::new(crate::channels::ChannelManager::new()); + channels + .add(Box::new(RecordingStatusChannel { + statuses: Arc::clone(&statuses), + })) + .await; + (channels, statuses) + }; + + struct StaticLlmProvider; + #[async_trait::async_trait] + impl ironclaw_llm::LlmProvider for StaticLlmProvider { + fn model_name(&self) -> &str { + "static-mock" + } + fn cost_per_token(&self) -> (Decimal, Decimal) { + (Decimal::ZERO, Decimal::ZERO) + } + async fn complete( + &self, + _request: ironclaw_llm::CompletionRequest, + ) -> Result { + unreachable!("LLM not invoked by this test") + } + async fn complete_with_tools( + &self, + _request: ironclaw_llm::ToolCompletionRequest, + ) -> Result { + unreachable!("LLM not invoked by this test") + } + } + + let deps = crate::agent::AgentDeps { + owner_id: "default".to_string(), + store: Some(Arc::clone(&store)), + settings_store: None, + llm: Arc::new(StaticLlmProvider), + cheap_llm: None, + safety: Arc::new(ironclaw_safety::SafetyLayer::new( + &ironclaw_safety::SafetyConfig { + max_output_length: 100_000, + injection_check_enabled: false, + }, + )), + tools: Arc::new(crate::tools::ToolRegistry::new()), + workspace: None, + extension_manager: None, + skill_registry: None, + skill_catalog: None, + skills_config: crate::config::SkillsConfig::default(), + hooks: Arc::new(crate::hooks::HookRegistry::new()), + auth_manager: None, + cost_guard: Arc::new(crate::agent::cost_guard::CostGuard::new( + crate::agent::cost_guard::CostGuardConfig::default(), + )), + sse_tx: None, + http_interceptor: None, + transcription: None, + document_extraction: None, + sandbox_readiness: crate::agent::routine_engine::SandboxReadiness::DisabledByConfig, + builder: None, + llm_backend: "nearai".to_string(), + tenant_rates: Arc::new(crate::tenant::TenantRateRegistry::new(4, 3)), + }; + let agent = Agent::new( + crate::config::AgentConfig { + name: "title-source-regression".to_string(), + max_parallel_jobs: 1, + job_timeout: Duration::from_secs(60), + stuck_threshold: Duration::from_secs(60), + repair_check_interval: Duration::from_secs(30), + max_repair_attempts: 1, + use_planning: false, + session_idle_timeout: Duration::from_secs(300), + allow_local_tools: false, + max_cost_per_day_cents: None, + max_actions_per_hour: None, + max_cost_per_user_per_day_cents: None, + max_tool_iterations: 5, + auto_approve_tools: false, + default_timezone: "UTC".to_string(), + max_jobs_per_user: None, + max_tokens_per_job: 0, + multi_tenant: false, + max_llm_concurrent_per_user: None, + max_jobs_concurrent_per_user: None, + engine_v2: false, + }, + deps, + channels, + None, + None, + None, + Some(Arc::new(crate::context::ContextManager::new(1))), + None, + ); + + // --- Case 1: attachment-augmented payload, real user text. + // Title must come from the raw text, not the augmented block. + let thread_a = Uuid::new_v4(); + store + .ensure_conversation(thread_a, "web", "user-1", None, Some("web")) + .await + .expect("ensure"); + let raw_a = "Summarise the attached report"; + let augmented_a = format!( + "{raw_a}\n\n\nQ3 revenue was $4.2M across\nthree product lines and margin compressed to 38%.\n\n" + ); + agent + .persist_user_message( + thread_a, + "web", + "user-1", + 1, + &augmented_a, + raw_a, + Utc::now(), + ) + .await + .expect("augmented-with-raw persist succeeds"); + let meta_a = store + .get_conversation_metadata(thread_a) + .await + .expect("meta") + .expect("exists"); + assert_eq!( + meta_a.get("title").and_then(|v| v.as_str()), + Some(raw_a), + "title must be derived from raw user text, not the augmented payload \ + (augmented_a would contain the block)" + ); + + // --- Case 2: attachment-only first turn (empty raw text). + // Title must remain unset so a later real-text turn can claim it. + let thread_b = Uuid::new_v4(); + store + .ensure_conversation(thread_b, "web", "user-2", None, Some("web")) + .await + .expect("ensure"); + let augmented_b = "\n[binary image]\n"; + agent + .persist_user_message(thread_b, "web", "user-2", 1, augmented_b, "", Utc::now()) + .await + .expect("attachment-only persist succeeds"); + let meta_b = store + .get_conversation_metadata(thread_b) + .await + .expect("meta") + .expect("exists"); + assert!( + meta_b + .get("title") + .and_then(|v| v.as_str()) + .map(|s| s.is_empty()) + .unwrap_or(true), + "attachment-only first turn must NOT claim the title slot, got {:?}", + meta_b.get("title") + ); + + // --- Case 3: plain-text first turn (no attachments). + // Raw == augmented; title should be set to that text. + let thread_c = Uuid::new_v4(); + store + .ensure_conversation(thread_c, "web", "user-3", None, Some("web")) + .await + .expect("ensure"); + let plain = "what's the weather in Paris today?"; + agent + .persist_user_message(thread_c, "web", "user-3", 1, plain, plain, Utc::now()) + .await + .expect("plain persist succeeds"); + let meta_c = store + .get_conversation_metadata(thread_c) + .await + .expect("meta") + .expect("exists"); + assert_eq!( + meta_c.get("title").and_then(|v| v.as_str()), + Some(plain), + "plain-text first turn should seed the title" + ); + } + /// Regression test for #1487: process_approval on a missing thread should error. #[tokio::test] async fn test_approval_on_missing_thread_should_error() { diff --git a/src/bridge/router.rs b/src/bridge/router.rs index 78d47128862..5bca81b34a5 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -4722,6 +4722,12 @@ async fn handle_with_engine_inner( &message.user_id, thread_config, validated_tz.as_ref().map(|tz| tz.name()), + // Raw content for title derivation. `effective_content` is the + // attachment-augmented payload (synthesized `` + // block / extracted OCR); the sidebar title must come from the + // raw user text, mirroring the v1 path in + // `thread_ops::persist_user_message`. + Some(content), extra_metadata, ) .await @@ -4792,9 +4798,27 @@ async fn handle_with_engine_inner( .ok() }; if let Some(cid) = v1_conv_id { - let _ = db + // Only set the sidebar title when the first user message + // actually persists — otherwise the sidebar can show a + // metadata title for a conversation whose first user row + // never landed (ghost-title regression). Mirrors the gating + // in `thread_ops::persist_user_message`. + // + // Title is derived from the raw user `content`, not the + // attachment-augmented `effective_content`, so an + // image-only or attachment-only first turn does not claim + // the title slot with the synthesized `` block + // or extracted attachment text. `set_title_if_missing` + // already skips empty/whitespace input, so an attachment- + // only turn simply leaves the title unset until a later + // turn carries real user text. + if db .add_conversation_message(cid, "user", effective_content) - .await; + .await + .is_ok() + { + crate::db::set_title_if_missing(db.as_ref(), cid, content).await; + } } } diff --git a/src/channels/web/features/chat/mod.rs b/src/channels/web/features/chat/mod.rs index 87f2a6f1781..6dabddcb35a 100644 --- a/src/channels/web/features/chat/mod.rs +++ b/src/channels/web/features/chat/mod.rs @@ -875,15 +875,24 @@ pub(crate) async fn chat_threads_handler( sorted_threads.sort_by_key(|t| std::cmp::Reverse(t.updated_at)); let threads: Vec = sorted_threads .into_iter() - .map(|t| ThreadInfo { - id: t.id, - state: thread_state_label(t.state).to_string(), - turn_count: t.turns.len(), - created_at: t.created_at.to_rfc3339(), - updated_at: t.updated_at.to_rfc3339(), - title: None, - thread_type: None, - channel: Some("gateway".to_string()), + .map(|t| { + // Derive the sidebar title from the raw user text when + // available. See `title_from_in_memory_turn` for rationale. + let title = t + .turns + .first() + .and_then(title_from_in_memory_turn) + .filter(|s: &String| !s.is_empty()); + ThreadInfo { + id: t.id, + state: thread_state_label(t.state).to_string(), + turn_count: t.turns.len(), + created_at: t.created_at.to_rfc3339(), + updated_at: t.updated_at.to_rfc3339(), + title, + thread_type: None, + channel: Some("gateway".to_string()), + } }) .collect(); @@ -1202,6 +1211,34 @@ fn in_progress_from_thread(thread: &crate::agent::session::Thread) -> Option` block as the conversation title. `user_input` is the +/// attachment-augmented payload fed to the LLM — only plain-text turns +/// can safely use it as the title source. Mirrors the title-derivation +/// rules used by the v1 DB path (`persist_user_message` / +/// `set_title_if_missing`). +fn title_from_in_memory_turn(turn: &crate::agent::session::Turn) -> Option { + let source = turn + .raw_user_input + .as_deref() + .unwrap_or(turn.user_input.as_str()); + let trimmed = source.trim(); + if trimmed.is_empty() { + return None; + } + let title: String = trimmed + .split_whitespace() + .collect::>() + .join(" ") + .chars() + .take(100) + .collect(); + Some(title) +} + fn thread_state_label(state: crate::agent::session::ThreadState) -> &'static str { match state { crate::agent::session::ThreadState::Idle => "Idle", @@ -1411,6 +1448,51 @@ mod tests { assert!(message.is_none()); } + #[test] + fn test_title_from_in_memory_turn_prefers_raw_input() { + // When raw_user_input is populated (attachment-augmented turn), + // the title must come from the raw text — not the augmented + // `user_input` that contains the synthesized `` + // block. Regression for Issue 1 of PR #2700. + let mut thread = crate::agent::session::Thread::new(Uuid::new_v4(), Some("gateway")); + thread.start_turn( + "Summarise the attached report\n\nQ3 rev $4.2M\n", + ); + { + let turn = thread.turns.last_mut().expect("turn"); + turn.raw_user_input = Some("Summarise the attached report".to_string()); + } + let title = title_from_in_memory_turn(&thread.turns[0]).expect("title"); + assert_eq!(title, "Summarise the attached report"); + } + + #[test] + fn test_title_from_in_memory_turn_falls_back_to_user_input() { + // Plain-text turns leave `raw_user_input` as None and `user_input` + // is already the raw text — the helper should use it. + let mut thread = crate::agent::session::Thread::new(Uuid::new_v4(), Some("gateway")); + thread.start_turn("what's the weather in Paris?"); + let title = title_from_in_memory_turn(&thread.turns[0]).expect("title"); + assert_eq!(title, "what's the weather in Paris?"); + } + + #[test] + fn test_title_from_in_memory_turn_none_for_empty_raw() { + // Attachment-only turn: raw_user_input == "" while user_input + // carries only the synthesized block. Returning None leaves the + // title unset so a later real-text turn can claim it, matching + // the v1 DB behaviour (`set_title_if_missing` skips empty input). + let mut thread = crate::agent::session::Thread::new(Uuid::new_v4(), Some("gateway")); + thread.start_turn( + "\n[binary]\n", + ); + { + let turn = thread.turns.last_mut().expect("turn"); + turn.raw_user_input = Some(String::new()); + } + assert!(title_from_in_memory_turn(&thread.turns[0]).is_none()); + } + #[test] fn test_engine_history_entry_uses_stable_id() { let thread_id = Uuid::new_v4(); diff --git a/src/db/libsql/conversations.rs b/src/db/libsql/conversations.rs index 3b673ba9212..dc5e4f50959 100644 --- a/src/db/libsql/conversations.rs +++ b/src/db/libsql/conversations.rs @@ -170,13 +170,23 @@ impl ConversationStore for LibSqlBackend { .and_then(|v| v.get("started_at")) .and_then(|v| v.as_str()) .map(String::from); - let sql_title = get_opt_text(&row, 6); - let title = sql_title.or_else(|| { - metadata - .get("routine_name") - .and_then(|v| v.as_str()) - .map(String::from) - }); + let sql_title = get_opt_text(&row, 6) + .map(|s| s.split_whitespace().collect::>().join(" ")) + .filter(|s| !s.is_empty()); + let title = sql_title + .or_else(|| { + metadata + .get("title") + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + .map(String::from) + }) + .or_else(|| { + metadata + .get("routine_name") + .and_then(|v| v.as_str()) + .map(String::from) + }); results.push(ConversationSummary { id: row .get::(0) @@ -249,13 +259,23 @@ impl ConversationStore for LibSqlBackend { .and_then(|v| v.get("started_at")) .and_then(|v| v.as_str()) .map(String::from); - let sql_title = get_opt_text(&row, 6); - let title = sql_title.or_else(|| { - metadata - .get("routine_name") - .and_then(|v| v.as_str()) - .map(String::from) - }); + let sql_title = get_opt_text(&row, 6) + .map(|s| s.split_whitespace().collect::>().join(" ")) + .filter(|s| !s.is_empty()); + let title = sql_title + .or_else(|| { + metadata + .get("title") + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + .map(String::from) + }) + .or_else(|| { + metadata + .get("routine_name") + .and_then(|v| v.as_str()) + .map(String::from) + }); results.push(ConversationSummary { id: row .get::(0) @@ -588,6 +608,39 @@ impl ConversationStore for LibSqlBackend { Ok(()) } + async fn set_conversation_title_if_empty( + &self, + id: Uuid, + title: &str, + ) -> Result { + let conn = self.connect().await?; + // Atomic check-and-write: patch the metadata only when `title` is + // currently missing, NULL, or empty. Expressed as a single + // conditional UPDATE so two concurrent callers cannot both observe + // an empty title and race to overwrite each other. + // + // `json_extract(metadata, '$.title')` returns NULL when the key is + // absent, and the `IS NULL OR = ''` pair covers the "no title yet" + // cases. When metadata itself is NULL (legacy rows), we initialise + // it with `{"title": ?}` via COALESCE. + let patch = serde_json::json!({ "title": title }).to_string(); + let affected = conn + .execute( + "UPDATE conversations \ + SET metadata = json_patch(COALESCE(metadata, '{}'), ?2) \ + WHERE id = ?1 \ + AND ( \ + metadata IS NULL \ + OR json_extract(metadata, '$.title') IS NULL \ + OR json_extract(metadata, '$.title') = '' \ + )", + params![id.to_string(), patch], + ) + .await + .map_err(|e| DatabaseError::Query(e.to_string()))?; + Ok(affected > 0) + } + async fn get_conversation_metadata( &self, id: Uuid, @@ -1009,4 +1062,187 @@ mod tests { "assistant thread lookup should backfill a legacy NULL source_channel" ); } + + /// Regression test for #2237: conversations with a metadata title should + /// use it as a fallback when the message-derived title is NULL. + #[tokio::test] + async fn test_metadata_title_used_as_fallback() { + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("test_metadata_title.db"); + let backend = LibSqlBackend::new_local(&db_path).await.unwrap(); + backend.run_migrations().await.unwrap(); + + let conv_id = Uuid::new_v4(); + let user_id = "user-title-test"; + + // Create a conversation with no messages + backend + .ensure_conversation(conv_id, "gateway", user_id, None, Some("gateway")) + .await + .unwrap(); + + // Set a metadata title (simulating what persist_user_message does) + let title_val = serde_json::json!("What is the weather today?"); + backend + .update_conversation_metadata_field(conv_id, "title", &title_val) + .await + .unwrap(); + + // List conversations -- title should come from metadata even without messages + let convs = backend + .list_conversations_all_channels(user_id, 50) + .await + .unwrap(); + + let conv = convs.iter().find(|c| c.id == conv_id).unwrap(); + assert_eq!( + conv.title.as_deref(), + Some("What is the weather today?"), + "Conversation title should fall back to metadata title when no user messages exist" + ); + } + + /// Regression test for #2237: message-derived title takes precedence over metadata. + #[tokio::test] + async fn test_message_title_takes_precedence_over_metadata() { + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("test_message_title_precedence.db"); + let backend = LibSqlBackend::new_local(&db_path).await.unwrap(); + backend.run_migrations().await.unwrap(); + + let conv_id = Uuid::new_v4(); + let user_id = "user-title-precedence"; + + backend + .ensure_conversation(conv_id, "gateway", user_id, None, Some("gateway")) + .await + .unwrap(); + + // Set metadata title + let title_val = serde_json::json!("metadata title"); + backend + .update_conversation_metadata_field(conv_id, "title", &title_val) + .await + .unwrap(); + + // Add a user message + backend + .add_conversation_message(conv_id, "user", "actual user message") + .await + .unwrap(); + + let convs = backend + .list_conversations_all_channels(user_id, 50) + .await + .unwrap(); + + let conv = convs.iter().find(|c| c.id == conv_id).unwrap(); + assert_eq!( + conv.title.as_deref(), + Some("actual user message"), + "Message-derived title should take precedence over metadata title" + ); + } + + /// Regression test for the check-then-update race in + /// `set_title_if_missing`. Two concurrent callers both observe an + /// empty title; only the first conditional UPDATE must land. + #[tokio::test] + async fn test_set_title_if_empty_is_atomic_under_concurrency() { + use std::sync::Arc; + + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("test_title_race.db"); + let backend = Arc::new(LibSqlBackend::new_local(&db_path).await.unwrap()); + backend.run_migrations().await.unwrap(); + + let conv_id = Uuid::new_v4(); + backend + .ensure_conversation(conv_id, "gateway", "racer", None, Some("gateway")) + .await + .unwrap(); + + let b1 = Arc::clone(&backend); + let b2 = Arc::clone(&backend); + let (r1, r2) = tokio::join!( + async move { + b1.set_conversation_title_if_empty(conv_id, "first title") + .await + }, + async move { + b2.set_conversation_title_if_empty(conv_id, "second title") + .await + } + ); + let won1 = r1.unwrap(); + let won2 = r2.unwrap(); + assert!( + won1 ^ won2, + "exactly one concurrent setter must win, got ({won1}, {won2})" + ); + + let meta = backend + .get_conversation_metadata(conv_id) + .await + .unwrap() + .unwrap(); + let title = meta.get("title").and_then(|v| v.as_str()).unwrap(); + assert!( + title == "first title" || title == "second title", + "title must be one of the two candidates, got {title:?}" + ); + + // A third attempt on an already-titled conv must NOT overwrite. + let won3 = backend + .set_conversation_title_if_empty(conv_id, "third title") + .await + .unwrap(); + assert!(!won3, "third attempt must be a no-op"); + let meta = backend + .get_conversation_metadata(conv_id) + .await + .unwrap() + .unwrap(); + let title_after = meta.get("title").and_then(|v| v.as_str()).unwrap(); + assert_eq!(title, title_after, "title must not change after first win"); + } + + /// Empty / NULL metadata must still be treated as "no title yet" so + /// a fresh conversation can claim the title slot on its first call. + #[tokio::test] + async fn test_set_title_if_empty_writes_when_unset() { + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("test_title_unset.db"); + let backend = LibSqlBackend::new_local(&db_path).await.unwrap(); + backend.run_migrations().await.unwrap(); + + let conv_id = Uuid::new_v4(); + backend + .ensure_conversation(conv_id, "gateway", "u", None, Some("gateway")) + .await + .unwrap(); + + let won = backend + .set_conversation_title_if_empty(conv_id, "hello") + .await + .unwrap(); + assert!(won); + + // Pre-populate with empty-string title (regression for the + // `= ''` branch of the WHERE clause). + let conv2 = Uuid::new_v4(); + backend + .ensure_conversation(conv2, "gateway", "u", None, Some("gateway")) + .await + .unwrap(); + backend + .update_conversation_metadata_field(conv2, "title", &serde_json::json!("")) + .await + .unwrap(); + let won_empty = backend + .set_conversation_title_if_empty(conv2, "real") + .await + .unwrap(); + assert!(won_empty, "empty-string title must be treated as unset"); + } } diff --git a/src/db/mod.rs b/src/db/mod.rs index 272245df731..92e412001ec 100644 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -478,6 +478,18 @@ pub trait ConversationStore: Send + Sync { &self, id: Uuid, ) -> Result, DatabaseError>; + /// Atomically set `metadata.title` only if it is currently missing or empty. + /// + /// Returns `true` if this call actually wrote the title, `false` if the + /// title was already set (or the conversation does not exist). Used to + /// close the check-then-update race in [`set_title_if_missing`] where + /// two concurrent writes could both observe an empty title and race to + /// write different values. + async fn set_conversation_title_if_empty( + &self, + id: Uuid, + title: &str, + ) -> Result; async fn list_conversation_messages( &self, conversation_id: Uuid, @@ -494,6 +506,44 @@ pub trait ConversationStore: Send + Sync { ) -> Result, DatabaseError>; } +/// Set a conversation title from user input if one hasn't been set yet. +/// +/// Skips empty/whitespace-only input so that image-only or attachment-only +/// messages don't permanently block title-setting with an empty string. +/// Truncates to the first 100 characters for sidebar display. +pub async fn set_title_if_missing( + store: &(dyn ConversationStore + Send + Sync), + conversation_id: Uuid, + user_input: &str, +) { + let trimmed = user_input.trim(); + if trimmed.is_empty() { + return; + } + + let title_text: String = trimmed + .split_whitespace() + .collect::>() + .join(" ") + .chars() + .take(100) + .collect(); + + // Atomic conditional update. Both backends only write when + // `metadata.title` is NULL, missing, or an empty string, so two + // concurrent first-turn writes cannot race to overwrite each other — + // the first to commit wins, subsequent calls are no-ops. + match store + .set_conversation_title_if_empty(conversation_id, &title_text) + .await + { + Ok(_) => {} + Err(e) => { + tracing::debug!("failed to atomically set title: {e}"); + } + } +} + #[async_trait] pub trait JobStore: Send + Sync { async fn save_job(&self, ctx: &JobContext) -> Result<(), DatabaseError>; diff --git a/src/db/postgres.rs b/src/db/postgres.rs index c7621bd8d62..f04ff0ec309 100644 --- a/src/db/postgres.rs +++ b/src/db/postgres.rs @@ -274,6 +274,14 @@ impl ConversationStore for PgBackend { self.store.get_conversation_metadata(id).await } + async fn set_conversation_title_if_empty( + &self, + id: Uuid, + title: &str, + ) -> Result { + self.store.set_conversation_title_if_empty(id, title).await + } + async fn list_conversation_messages( &self, conversation_id: Uuid, diff --git a/src/history/store.rs b/src/history/store.rs index 7823bf8c302..0a4f167e505 100644 --- a/src/history/store.rs +++ b/src/history/store.rs @@ -1838,13 +1838,24 @@ impl Store { .and_then(|v| v.get("started_at")) .and_then(|v| v.as_str()) .map(String::from); - let sql_title: Option = r.get("title"); - let title = sql_title.or_else(|| { - metadata - .get("routine_name") - .and_then(|v| v.as_str()) - .map(String::from) - }); + let sql_title: Option = r + .get::<_, Option>("title") + .map(|s| s.split_whitespace().collect::>().join(" ")) + .filter(|s| !s.is_empty()); + let title = sql_title + .or_else(|| { + metadata + .get("title") + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + .map(String::from) + }) + .or_else(|| { + metadata + .get("routine_name") + .and_then(|v| v.as_str()) + .map(String::from) + }); ConversationSummary { id: r.get("id"), title, @@ -1912,13 +1923,24 @@ impl Store { .map(String::from); // For routine/heartbeat threads, derive title from metadata // since they may have no user messages. - let sql_title: Option = r.get("title"); - let title = sql_title.or_else(|| { - metadata - .get("routine_name") - .and_then(|v| v.as_str()) - .map(String::from) - }); + let sql_title: Option = r + .get::<_, Option>("title") + .map(|s| s.split_whitespace().collect::>().join(" ")) + .filter(|s| !s.is_empty()); + let title = sql_title + .or_else(|| { + metadata + .get("title") + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + .map(String::from) + }) + .or_else(|| { + metadata + .get("routine_name") + .and_then(|v| v.as_str()) + .map(String::from) + }); ConversationSummary { id: r.get("id"), title, @@ -2228,6 +2250,37 @@ impl Store { Ok(()) } + /// Atomically set `metadata.title` only when it's missing or empty. + /// + /// Returns `true` if the write actually landed (this caller "won" the + /// race), `false` if the title was already set or the conversation does + /// not exist. Expressed as a single conditional UPDATE so two concurrent + /// first-turn writes cannot both observe an empty title and race to + /// overwrite each other. + pub async fn set_conversation_title_if_empty( + &self, + id: Uuid, + title: &str, + ) -> Result { + let conn = self.conn().await?; + let patch = serde_json::json!({ "title": title }); + let affected = conn + .execute( + "UPDATE conversations \ + SET metadata = COALESCE(metadata, '{}'::jsonb) || $2 \ + WHERE id = $1 \ + AND ( \ + metadata IS NULL \ + OR NOT (metadata ? 'title') \ + OR metadata->>'title' IS NULL \ + OR metadata->>'title' = '' \ + )", + &[&id, &patch], + ) + .await?; + Ok(affected > 0) + } + /// Read the metadata JSONB for a conversation. pub async fn get_conversation_metadata( &self, @@ -3392,4 +3445,202 @@ mod tests { .await .unwrap(); } + + /// PG mirror of `src/db/libsql/conversations.rs` + /// `test_metadata_title_used_as_fallback`. + /// + /// Regression for #2237: when no user messages have been persisted, + /// `list_conversations_all_channels` must fall back to the + /// `metadata.title` field that `set_title_if_missing` writes. The + /// libSQL test pins this for the embedded path; this mirror pins + /// the PostgreSQL path so a regression in the PG SQL (e.g. JSON + /// extraction operator drift, precedence error in the `COALESCE` / + /// `UNION` rewrite) surfaces at the integration tier instead of in + /// production. + /// + /// Integration tier — ignored by default. Requires a reachable + /// PostgreSQL with migrations applied. Run with: + /// + /// ```text + /// cargo test -p ironclaw --features integration --lib \ + /// history::store::tests::test_metadata_title_used_as_fallback_pg -- --ignored + /// ``` + #[cfg(feature = "postgres")] + #[tokio::test] + #[ignore] + async fn test_metadata_title_used_as_fallback_pg() { + use crate::config::Config; + + let _ = dotenvy::dotenv(); + let config = Config::from_env().await.expect("Failed to load config"); + let store = Store::new(&config.database) + .await + .expect("Failed to connect to database"); + store + .run_migrations() + .await + .expect("Failed to run migrations"); + + let conv_id = Uuid::new_v4(); + let user_id = format!("pg-title-fallback-{}", Uuid::new_v4()); + + store + .ensure_conversation(conv_id, "gateway", &user_id, None, Some("gateway")) + .await + .unwrap(); + + let title_val = serde_json::json!("What is the weather today?"); + store + .update_conversation_metadata_field(conv_id, "title", &title_val) + .await + .unwrap(); + + let convs = store + .list_conversations_all_channels(&user_id, 50) + .await + .unwrap(); + let conv = convs.iter().find(|c| c.id == conv_id).expect("conv listed"); + assert_eq!( + conv.title.as_deref(), + Some("What is the weather today?"), + "PG list must fall back to metadata.title when no user messages exist" + ); + + let conn = store.conn().await.unwrap(); + conn.execute( + "DELETE FROM conversations WHERE id = $1", + &[&conv_id.to_string()], + ) + .await + .unwrap(); + } + + /// PG mirror of `src/db/libsql/conversations.rs` + /// `test_message_title_takes_precedence_over_metadata`. + /// + /// Pins the precedence order: once a first user message is + /// persisted, the derived title (from the message body) takes over + /// and the `metadata.title` set at conversation creation is + /// shadowed. Protects against a regression where the PG SQL + /// accidentally prefers `metadata.title` even when a message row + /// exists. + #[cfg(feature = "postgres")] + #[tokio::test] + #[ignore] + async fn test_message_title_takes_precedence_over_metadata_pg() { + use crate::config::Config; + + let _ = dotenvy::dotenv(); + let config = Config::from_env().await.expect("Failed to load config"); + let store = Store::new(&config.database) + .await + .expect("Failed to connect to database"); + store + .run_migrations() + .await + .expect("Failed to run migrations"); + + let conv_id = Uuid::new_v4(); + let user_id = format!("pg-title-precedence-{}", Uuid::new_v4()); + + store + .ensure_conversation(conv_id, "gateway", &user_id, None, Some("gateway")) + .await + .unwrap(); + + let title_val = serde_json::json!("metadata title"); + store + .update_conversation_metadata_field(conv_id, "title", &title_val) + .await + .unwrap(); + + store + .add_conversation_message(conv_id, "user", "actual user message") + .await + .unwrap(); + + let convs = store + .list_conversations_all_channels(&user_id, 50) + .await + .unwrap(); + let conv = convs.iter().find(|c| c.id == conv_id).expect("conv listed"); + assert_eq!( + conv.title.as_deref(), + Some("actual user message"), + "first user message must shadow metadata.title on the PG path" + ); + + let conn = store.conn().await.unwrap(); + conn.execute( + "DELETE FROM conversation_messages WHERE conversation_id = $1", + &[&conv_id.to_string()], + ) + .await + .unwrap(); + conn.execute( + "DELETE FROM conversations WHERE id = $1", + &[&conv_id.to_string()], + ) + .await + .unwrap(); + } + + /// PG regression for the atomic title write. + /// + /// Two concurrent callers both observe an empty title; exactly one + /// conditional UPDATE must land. Subsequent calls on an + /// already-titled conversation must be no-ops. Integration tier — + /// ignored by default. Run with: + /// + /// ```text + /// cargo test -p ironclaw --features integration --lib \ + /// history::store::tests::test_set_title_if_empty_is_atomic_pg -- --ignored + /// ``` + #[cfg(feature = "postgres")] + #[tokio::test] + #[ignore] + async fn test_set_title_if_empty_is_atomic_pg() { + use crate::config::Config; + use std::sync::Arc; + + let _ = dotenvy::dotenv(); + let config = Config::from_env().await.expect("load config"); + let store = Arc::new(Store::new(&config.database).await.expect("connect")); + store.run_migrations().await.expect("migrate"); + + let conv_id = Uuid::new_v4(); + let user_id = format!("pg-title-race-{}", Uuid::new_v4()); + store + .ensure_conversation(conv_id, "gateway", &user_id, None, Some("gateway")) + .await + .unwrap(); + + let s1 = Arc::clone(&store); + let s2 = Arc::clone(&store); + let (r1, r2) = tokio::join!( + async move { s1.set_conversation_title_if_empty(conv_id, "first").await }, + async move { s2.set_conversation_title_if_empty(conv_id, "second").await } + ); + let won1 = r1.unwrap(); + let won2 = r2.unwrap(); + assert!( + won1 ^ won2, + "exactly one concurrent setter must win, got ({won1}, {won2})" + ); + + // Third attempt on titled conv must be a no-op. + let won3 = store + .set_conversation_title_if_empty(conv_id, "third") + .await + .unwrap(); + assert!(!won3, "must not overwrite an existing title"); + + let conn = store.conn().await.unwrap(); + conn.execute( + "DELETE FROM conversations WHERE id = $1", + &[&conv_id.to_string()], + ) + .await + .unwrap(); + } } diff --git a/src/hooks/session_summary.rs b/src/hooks/session_summary.rs index 67fd8bb8b62..53b65833235 100644 --- a/src/hooks/session_summary.rs +++ b/src/hooks/session_summary.rs @@ -350,6 +350,14 @@ mod tests { unimplemented!() } + async fn set_conversation_title_if_empty( + &self, + _id: Uuid, + _title: &str, + ) -> Result { + unimplemented!() + } + async fn list_conversation_messages( &self, _conversation_id: Uuid,