agent: Handle thread title generation errors (#30273)

The title of a (text) thread would get stuck in "Loading Summary..."
when the request to generate it failed. We now handle this case by
falling back to the default title, and letting the user manually edit
the title or retry generating it.



https://github.com/user-attachments/assets/898d26ad-d31f-4b62-9b05-519d923b1b22



Release Notes:

- agent: Handle thread title generation errors

---------

Co-authored-by: Richard Feldman <oss@rtfeldman.com>

crates/assistant_context_editor/src/context.rs

@@ -1,7 +1,7 @@
 #[cfg(test)]
 mod context_tests;
 
-use anyhow::{Context as _, Result, anyhow};
+use anyhow::{Context as _, Result, anyhow, bail};
 use assistant_settings::AssistantSettings;
 use assistant_slash_command::{
     SlashCommandContent, SlashCommandEvent, SlashCommandLine, SlashCommandOutputSection,
@@ -133,7 +133,7 @@ pub enum ContextOperation {
         version: clock::Global,
     },
     UpdateSummary {
-        summary: ContextSummary,
+        summary: ContextSummaryContent,
         version: clock::Global,
     },
     SlashCommandStarted {
@@ -203,7 +203,7 @@ impl ContextOperation {
                 version: language::proto::deserialize_version(&update.version),
             }),
             proto::context_operation::Variant::UpdateSummary(update) => Ok(Self::UpdateSummary {
-                summary: ContextSummary {
+                summary: ContextSummaryContent {
                     text: update.summary,
                     done: update.done,
                     timestamp: language::proto::deserialize_timestamp(
@@ -467,11 +467,73 @@ pub enum ContextEvent {
     Operation(ContextOperation),
 }
 
-#[derive(Clone, Default, Debug)]
-pub struct ContextSummary {
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub enum ContextSummary {
+    Pending,
+    Content(ContextSummaryContent),
+    Error,
+}
+
+#[derive(Default, Clone, Debug, Eq, PartialEq)]
+pub struct ContextSummaryContent {
     pub text: String,
     pub done: bool,
-    timestamp: clock::Lamport,
+    pub timestamp: clock::Lamport,
+}
+
+impl ContextSummary {
+    pub const DEFAULT: &str = "New Text Thread";
+
+    pub fn or_default(&self) -> SharedString {
+        self.unwrap_or(Self::DEFAULT)
+    }
+
+    pub fn unwrap_or(&self, message: impl Into<SharedString>) -> SharedString {
+        self.content()
+            .map_or_else(|| message.into(), |content| content.text.clone().into())
+    }
+
+    pub fn content(&self) -> Option<&ContextSummaryContent> {
+        match self {
+            ContextSummary::Content(content) => Some(content),
+            ContextSummary::Pending | ContextSummary::Error => None,
+        }
+    }
+
+    fn content_as_mut(&mut self) -> Option<&mut ContextSummaryContent> {
+        match self {
+            ContextSummary::Content(content) => Some(content),
+            ContextSummary::Pending | ContextSummary::Error => None,
+        }
+    }
+
+    fn content_or_set_empty(&mut self) -> &mut ContextSummaryContent {
+        match self {
+            ContextSummary::Content(content) => content,
+            ContextSummary::Pending | ContextSummary::Error => {
+                let content = ContextSummaryContent::default();
+                *self = ContextSummary::Content(content);
+                self.content_as_mut().unwrap()
+            }
+        }
+    }
+
+    pub fn is_pending(&self) -> bool {
+        matches!(self, ContextSummary::Pending)
+    }
+
+    fn timestamp(&self) -> Option<clock::Lamport> {
+        match self {
+            ContextSummary::Content(content) => Some(content.timestamp),
+            ContextSummary::Pending | ContextSummary::Error => None,
+        }
+    }
+}
+
+impl PartialOrd for ContextSummary {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        self.timestamp().partial_cmp(&other.timestamp())
+    }
 }
 
 #[derive(Clone, Debug, Eq, PartialEq)]
@@ -607,7 +669,7 @@ pub struct AssistantContext {
     message_anchors: Vec<MessageAnchor>,
     contents: Vec<Content>,
     messages_metadata: HashMap<MessageId, MessageMetadata>,
-    summary: Option<ContextSummary>,
+    summary: ContextSummary,
     summary_task: Task<Option<()>>,
     completion_count: usize,
     pending_completions: Vec<PendingCompletion>,
@@ -694,7 +756,7 @@ impl AssistantContext {
             slash_command_output_sections: Vec::new(),
             thought_process_output_sections: Vec::new(),
             edits_since_last_parse: edits_since_last_slash_command_parse,
-            summary: None,
+            summary: ContextSummary::Pending,
             summary_task: Task::ready(None),
             completion_count: Default::default(),
             pending_completions: Default::default(),
@@ -753,7 +815,7 @@ impl AssistantContext {
                 .collect(),
             summary: self
                 .summary
-                .as_ref()
+                .content()
                 .map(|summary| summary.text.clone())
                 .unwrap_or_default(),
             slash_command_output_sections: self
@@ -939,12 +1001,10 @@ impl AssistantContext {
                     summary: new_summary,
                     ..
                 } => {
-                    if self
-                        .summary
-                        .as_ref()
-                        .map_or(true, |summary| new_summary.timestamp > summary.timestamp)
-                    {
-                        self.summary = Some(new_summary);
+                    if self.summary.timestamp().map_or(true, |current_timestamp| {
+                        new_summary.timestamp > current_timestamp
+                    }) {
+                        self.summary = ContextSummary::Content(new_summary);
                         summary_generated = true;
                     }
                 }
@@ -1102,8 +1162,8 @@ impl AssistantContext {
         self.path.as_ref()
     }
 
-    pub fn summary(&self) -> Option<&ContextSummary> {
-        self.summary.as_ref()
+    pub fn summary(&self) -> &ContextSummary {
+        &self.summary
     }
 
     pub fn parsed_slash_commands(&self) -> &[ParsedSlashCommand] {
@@ -2576,7 +2636,7 @@ impl AssistantContext {
             return;
         };
 
-        if replace_old || (self.message_anchors.len() >= 2 && self.summary.is_none()) {
+        if replace_old || (self.message_anchors.len() >= 2 && self.summary.is_pending()) {
             if !model.provider.is_authenticated(cx) {
                 return;
             }
@@ -2593,17 +2653,20 @@ impl AssistantContext {
 
             // If there is no summary, it is set with `done: false` so that "Loading Summary…" can
             // be displayed.
-            if self.summary.is_none() {
-                self.summary = Some(ContextSummary {
-                    text: "".to_string(),
-                    done: false,
-                    timestamp: clock::Lamport::default(),
-                });
-                replace_old = true;
+            match self.summary {
+                ContextSummary::Pending | ContextSummary::Error => {
+                    self.summary = ContextSummary::Content(ContextSummaryContent {
+                        text: "".to_string(),
+                        done: false,
+                        timestamp: clock::Lamport::default(),
+                    });
+                    replace_old = true;
+                }
+                ContextSummary::Content(_) => {}
             }
 
             self.summary_task = cx.spawn(async move |this, cx| {
-                async move {
+                let result = async {
                     let stream = model.model.stream_completion_text(request, &cx);
                     let mut messages = stream.await?;
 
@@ -2614,7 +2677,7 @@ impl AssistantContext {
                         this.update(cx, |this, cx| {
                             let version = this.version.clone();
                             let timestamp = this.next_timestamp();
-                            let summary = this.summary.get_or_insert(ContextSummary::default());
+                            let summary = this.summary.content_or_set_empty();
                             if !replaced && replace_old {
                                 summary.text.clear();
                                 replaced = true;
@@ -2636,10 +2699,19 @@ impl AssistantContext {
                         }
                     }
 
+                    this.read_with(cx, |this, _cx| {
+                        if let Some(summary) = this.summary.content() {
+                            if summary.text.is_empty() {
+                                bail!("Model generated an empty summary");
+                            }
+                        }
+                        Ok(())
+                    })??;
+
                     this.update(cx, |this, cx| {
                         let version = this.version.clone();
                         let timestamp = this.next_timestamp();
-                        if let Some(summary) = this.summary.as_mut() {
+                        if let Some(summary) = this.summary.content_as_mut() {
                             summary.done = true;
                             summary.timestamp = timestamp;
                             let operation = ContextOperation::UpdateSummary {
@@ -2654,8 +2726,18 @@ impl AssistantContext {
 
                     anyhow::Ok(())
                 }
-                .log_err()
-                .await
+                .await;
+
+                if let Err(err) = result {
+                    this.update(cx, |this, cx| {
+                        this.summary = ContextSummary::Error;
+                        cx.emit(ContextEvent::SummaryChanged);
+                    })
+                    .log_err();
+                    log::error!("Error generating context summary: {}", err);
+                }
+
+                Some(())
             });
         }
     }
@@ -2769,7 +2851,7 @@ impl AssistantContext {
 
             let (old_path, summary) = this.read_with(cx, |this, _| {
                 let path = this.path.clone();
-                let summary = if let Some(summary) = this.summary.as_ref() {
+                let summary = if let Some(summary) = this.summary.content() {
                     if summary.done {
                         Some(summary.text.clone())
                     } else {
@@ -2823,21 +2905,12 @@ impl AssistantContext {
 
     pub fn set_custom_summary(&mut self, custom_summary: String, cx: &mut Context<Self>) {
         let timestamp = self.next_timestamp();
-        let summary = self.summary.get_or_insert(ContextSummary::default());
+        let summary = self.summary.content_or_set_empty();
         summary.timestamp = timestamp;
         summary.done = true;
         summary.text = custom_summary;
         cx.emit(ContextEvent::SummaryChanged);
     }
-
-    pub const DEFAULT_SUMMARY: SharedString = SharedString::new_static("New Text Thread");
-
-    pub fn summary_or_default(&self) -> SharedString {
-        self.summary
-            .as_ref()
-            .map(|summary| summary.text.clone().into())
-            .unwrap_or(Self::DEFAULT_SUMMARY)
-    }
 }
 
 #[derive(Debug, Default)]
@@ -3053,7 +3126,7 @@ impl SavedContext {
 
         let timestamp = next_timestamp.tick();
         operations.push(ContextOperation::UpdateSummary {
-            summary: ContextSummary {
+            summary: ContextSummaryContent {
                 text: self.summary,
                 done: true,
                 timestamp,

crates/assistant_context_editor/src/context/context_tests.rs

@@ -1,5 +1,5 @@
 use crate::{
-    AssistantContext, CacheStatus, ContextEvent, ContextId, ContextOperation,
+    AssistantContext, CacheStatus, ContextEvent, ContextId, ContextOperation, ContextSummary,
     InvokedSlashCommandId, MessageCacheMetadata, MessageId, MessageStatus,
 };
 use anyhow::Result;
@@ -16,7 +16,10 @@ use futures::{
 };
 use gpui::{App, Entity, SharedString, Task, TestAppContext, WeakEntity, prelude::*};
 use language::{Buffer, BufferSnapshot, LanguageRegistry, LspAdapterDelegate};
-use language_model::{LanguageModelCacheConfiguration, LanguageModelRegistry, Role};
+use language_model::{
+    ConfiguredModel, LanguageModelCacheConfiguration, LanguageModelRegistry, Role,
+    fake_provider::{FakeLanguageModel, FakeLanguageModelProvider},
+};
 use parking_lot::Mutex;
 use pretty_assertions::assert_eq;
 use project::Project;
@@ -1177,6 +1180,187 @@ fn test_mark_cache_anchors(cx: &mut App) {
     );
 }
 
+#[gpui::test]
+async fn test_summarization(cx: &mut TestAppContext) {
+    let (context, fake_model) = setup_context_editor_with_fake_model(cx);
+
+    // Initial state should be pending
+    context.read_with(cx, |context, _| {
+        assert!(matches!(context.summary(), ContextSummary::Pending));
+        assert_eq!(context.summary().or_default(), ContextSummary::DEFAULT);
+    });
+
+    let message_1 = context.read_with(cx, |context, _cx| context.message_anchors[0].clone());
+    context.update(cx, |context, cx| {
+        context
+            .insert_message_after(message_1.id, Role::Assistant, MessageStatus::Done, cx)
+            .unwrap();
+    });
+
+    // Send a message
+    context.update(cx, |context, cx| {
+        context.assist(cx);
+    });
+
+    simulate_successful_response(&fake_model, cx);
+
+    // Should start generating summary when there are >= 2 messages
+    context.read_with(cx, |context, _| {
+        assert!(!context.summary().content().unwrap().done);
+    });
+
+    cx.run_until_parked();
+    fake_model.stream_last_completion_response("Brief".into());
+    fake_model.stream_last_completion_response(" Introduction".into());
+    fake_model.end_last_completion_stream();
+    cx.run_until_parked();
+
+    // Summary should be set
+    context.read_with(cx, |context, _| {
+        assert_eq!(context.summary().or_default(), "Brief Introduction");
+    });
+
+    // We should be able to manually set a summary
+    context.update(cx, |context, cx| {
+        context.set_custom_summary("Brief Intro".into(), cx);
+    });
+
+    context.read_with(cx, |context, _| {
+        assert_eq!(context.summary().or_default(), "Brief Intro");
+    });
+}
+
+#[gpui::test]
+async fn test_thread_summary_error_set_manually(cx: &mut TestAppContext) {
+    let (context, fake_model) = setup_context_editor_with_fake_model(cx);
+
+    test_summarize_error(&fake_model, &context, cx);
+
+    // Now we should be able to set a summary
+    context.update(cx, |context, cx| {
+        context.set_custom_summary("Brief Intro".into(), cx);
+    });
+
+    context.read_with(cx, |context, _| {
+        assert_eq!(context.summary().or_default(), "Brief Intro");
+    });
+}
+
+#[gpui::test]
+async fn test_thread_summary_error_retry(cx: &mut TestAppContext) {
+    let (context, fake_model) = setup_context_editor_with_fake_model(cx);
+
+    test_summarize_error(&fake_model, &context, cx);
+
+    // Sending another message should not trigger another summarize request
+    context.update(cx, |context, cx| {
+        context.assist(cx);
+    });
+
+    simulate_successful_response(&fake_model, cx);
+
+    context.read_with(cx, |context, _| {
+        // State is still Error, not Generating
+        assert!(matches!(context.summary(), ContextSummary::Error));
+    });
+
+    // But the summarize request can be invoked manually
+    context.update(cx, |context, cx| {
+        context.summarize(true, cx);
+    });
+
+    context.read_with(cx, |context, _| {
+        assert!(!context.summary().content().unwrap().done);
+    });
+
+    cx.run_until_parked();
+    fake_model.stream_last_completion_response("A successful summary".into());
+    fake_model.end_last_completion_stream();
+    cx.run_until_parked();
+
+    context.read_with(cx, |context, _| {
+        assert_eq!(context.summary().or_default(), "A successful summary");
+    });
+}
+
+fn test_summarize_error(
+    model: &Arc<FakeLanguageModel>,
+    context: &Entity<AssistantContext>,
+    cx: &mut TestAppContext,
+) {
+    let message_1 = context.read_with(cx, |context, _cx| context.message_anchors[0].clone());
+    context.update(cx, |context, cx| {
+        context
+            .insert_message_after(message_1.id, Role::Assistant, MessageStatus::Done, cx)
+            .unwrap();
+    });
+
+    // Send a message
+    context.update(cx, |context, cx| {
+        context.assist(cx);
+    });
+
+    simulate_successful_response(&model, cx);
+
+    context.read_with(cx, |context, _| {
+        assert!(!context.summary().content().unwrap().done);
+    });
+
+    // Simulate summary request ending
+    cx.run_until_parked();
+    model.end_last_completion_stream();
+    cx.run_until_parked();
+
+    // State is set to Error and default message
+    context.read_with(cx, |context, _| {
+        assert_eq!(*context.summary(), ContextSummary::Error);
+        assert_eq!(context.summary().or_default(), ContextSummary::DEFAULT);
+    });
+}
+
+fn setup_context_editor_with_fake_model(
+    cx: &mut TestAppContext,
+) -> (Entity<AssistantContext>, Arc<FakeLanguageModel>) {
+    let registry = Arc::new(LanguageRegistry::test(cx.executor().clone()));
+
+    let fake_provider = Arc::new(FakeLanguageModelProvider);
+    let fake_model = Arc::new(fake_provider.test_model());
+
+    cx.update(|cx| {
+        init_test(cx);
+        LanguageModelRegistry::global(cx).update(cx, |registry, cx| {
+            registry.set_default_model(
+                Some(ConfiguredModel {
+                    provider: fake_provider.clone(),
+                    model: fake_model.clone(),
+                }),
+                cx,
+            )
+        })
+    });
+
+    let prompt_builder = Arc::new(PromptBuilder::new(None).unwrap());
+    let context = cx.new(|cx| {
+        AssistantContext::local(
+            registry,
+            None,
+            None,
+            prompt_builder.clone(),
+            Arc::new(SlashCommandWorkingSet::default()),
+            cx,
+        )
+    });
+
+    (context, fake_model)
+}
+
+fn simulate_successful_response(fake_model: &FakeLanguageModel, cx: &mut TestAppContext) {
+    cx.run_until_parked();
+    fake_model.stream_last_completion_response("Assistant response".into());
+    fake_model.end_last_completion_stream();
+    cx.run_until_parked();
+}
+
 fn messages(context: &Entity<AssistantContext>, cx: &App) -> Vec<(MessageId, Role, Range<usize>)> {
     context
         .read(cx)

crates/assistant_context_editor/src/context_editor.rs

@@ -1860,7 +1860,12 @@ impl ContextEditor {
     }
 
     pub fn title(&self, cx: &App) -> SharedString {
-        self.context.read(cx).summary_or_default()
+        self.context.read(cx).summary().or_default()
+    }
+
+    pub fn regenerate_summary(&mut self, cx: &mut Context<Self>) {
+        self.context
+            .update(cx, |context, cx| context.summarize(true, cx));
     }
 
     fn render_notice(&self, cx: &mut Context<Self>) -> Option<AnyElement> {

crates/assistant_context_editor/src/context_store.rs

@@ -648,7 +648,10 @@ impl ContextStore {
                 if context.replica_id() == ReplicaId::default() {
                     Some(proto::ContextMetadata {
                         context_id: context.id().to_proto(),
-                        summary: context.summary().map(|summary| summary.text.clone()),
+                        summary: context
+                            .summary()
+                            .content()
+                            .map(|summary| summary.text.clone()),
                     })
                 } else {
                     None