From 6646a0f6e2195d1f9979d8d0165a5efdc37d8928 Mon Sep 17 00:00:00 2001 From: Douwe Osinga Date: Wed, 5 Aug 2026 17:36:47 +0200 Subject: [PATCH] fix: keep ACP session naming out of live conversations (#10963) Co-authored-by: Douwe M Osinga --- crates/goose-provider-types/src/base.rs | 3 + crates/goose/src/acp/provider.rs | 58 ++++++++ crates/goose/src/agents/agent.rs | 23 ++++ crates/goose/src/session/session_manager.rs | 138 +++++++++++++++++++- crates/goose/src/session/session_naming.rs | 25 ++-- 5 files changed, 237 insertions(+), 10 deletions(-) diff --git a/crates/goose-provider-types/src/base.rs b/crates/goose-provider-types/src/base.rs index c21cf3117..beee620a4 100644 --- a/crates/goose-provider-types/src/base.rs +++ b/crates/goose-provider-types/src/base.rs @@ -5,6 +5,7 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use std::collections::HashMap; use std::pin::Pin; +use std::sync::Arc; use crate::{ canonical::{map_to_canonical_model, CanonicalModelRegistry}, @@ -582,6 +583,8 @@ pub trait Provider: Send + Sync { false } + fn set_session_title_callback(&self, _callback: Arc) {} + /// Configure OAuth authentication for this provider /// /// This method is called when a provider has configuration keys marked with oauth_flow = true. diff --git a/crates/goose/src/acp/provider.rs b/crates/goose/src/acp/provider.rs index cbddeac50..ec3d4e193 100644 --- a/crates/goose/src/acp/provider.rs +++ b/crates/goose/src/acp/provider.rs @@ -142,6 +142,30 @@ struct HandoffContextClaim { include_context: bool, } +type SessionTitleCallback = Arc; + +#[derive(Clone, Default)] +struct SessionTitlePublisher { + callback: Arc>>, +} + +impl SessionTitlePublisher { + fn set_callback(&self, callback: SessionTitleCallback) { + *self.callback.lock().unwrap() = Some(callback); + } + + fn publish(&self, title: &str) { + let title = title.trim(); + if title.is_empty() { + return; + } + + if let Some(callback) = self.callback.lock().unwrap().clone() { + callback(title.to_string()); + } + } +} + pub struct AcpProvider { name: String, goose_mode: Arc>, @@ -158,6 +182,7 @@ pub struct AcpProvider { /// in which case `get_context_limit()` falls back to the supplied model /// configuration's context limit. context_size: Arc, + session_title_publisher: SessionTitlePublisher, /// Config option id used to select the model, if this agent supports it. model_config_option_id: Option, @@ -245,11 +270,13 @@ impl AcpProvider { let pending_tool_updates: Arc>> = Arc::new(Mutex::new(HashMap::new())); let context_size = Arc::new(AtomicU64::new(0)); + let session_title_publisher = SessionTitlePublisher::default(); let client_loop = AcpClientLoop::new( config, goose_mode_shared.clone(), pending_tool_updates.clone(), context_size.clone(), + session_title_publisher.clone(), ); let loop_thread = spawn_client_loop(run(client_loop, rx, init_tx)); @@ -282,6 +309,7 @@ impl AcpProvider { pending_tool_updates, handoff_context_sent: AtomicBool::new(false), context_size, + session_title_publisher, model_config_option_id, applied_model: Arc::new(Mutex::new(applied_model)), tx: Some(tx), @@ -459,6 +487,10 @@ impl Provider for AcpProvider { true } + fn set_session_title_callback(&self, callback: Arc) { + self.session_title_publisher.set_callback(callback); + } + async fn handle_permission_confirmation( &self, request_id: &str, @@ -698,6 +730,7 @@ struct AcpClientLoop { prompt_response_tx: Arc>>>, pending_tool_updates: Arc>>, context_size: Arc, + session_title_publisher: SessionTitlePublisher, } impl AcpClientLoop { @@ -706,6 +739,7 @@ impl AcpClientLoop { goose_mode: Arc>, pending_tool_updates: Arc>>, context_size: Arc, + session_title_publisher: SessionTitlePublisher, ) -> Self { Self { config, @@ -713,6 +747,7 @@ impl AcpClientLoop { prompt_response_tx: Arc::new(Mutex::new(None)), pending_tool_updates, context_size, + session_title_publisher, } } @@ -767,6 +802,7 @@ impl AcpClientLoop { prompt_response_tx, pending_tool_updates, context_size, + session_title_publisher, } = self; let notification_callback = config.notification_callback.clone(); let reverse_modes = reverse_mode_mapping(&config.mode_mapping); @@ -780,6 +816,7 @@ impl AcpClientLoop { let goose_mode = goose_mode.clone(); let pending_tool_updates = pending_tool_updates.clone(); let context_size = context_size.clone(); + let session_title_publisher = session_title_publisher.clone(); async move |notification: SessionNotification, _cx| { if let Some(ref cb) = notification_callback { cb(notification.clone()); @@ -816,6 +853,11 @@ impl AcpClientLoop { SessionUpdate::UsageUpdate(usage) => { context_size.store(usage.size, Ordering::Relaxed); } + SessionUpdate::SessionInfoUpdate(update) => { + if let Some(title) = update.title.value() { + session_title_publisher.publish(title); + } + } _ => {} } if let Some(tx) = prompt_response_tx @@ -1686,6 +1728,21 @@ mod tests { test_provider_with_tx(None) } + #[test] + fn session_title_publisher_forwards_non_empty_titles() { + let publisher = SessionTitlePublisher::default(); + let titles = Arc::new(Mutex::new(Vec::new())); + let received = titles.clone(); + publisher.set_callback(Arc::new(move |title| { + received.lock().unwrap().push(title); + })); + + publisher.publish(" Generated title "); + publisher.publish(" "); + + assert_eq!(*titles.lock().unwrap(), vec!["Generated title"]); + } + fn test_provider_with_tx( tx: Option>, ) -> (AcpProvider, ModelConfig) { @@ -1702,6 +1759,7 @@ mod tests { pending_tool_updates: Arc::new(Mutex::new(HashMap::new())), handoff_context_sent: AtomicBool::new(false), context_size: Arc::new(AtomicU64::new(0)), + session_title_publisher: SessionTitlePublisher::default(), model_config_option_id: None, applied_model: Arc::new(Mutex::new(None)), tx, diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index beac05982..eb6c6548e 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -3182,6 +3182,29 @@ impl Agent { session_id: &str, ) -> Result<()> { let provider_name = provider.get_name().to_string(); + let session_manager = self.config.session_manager.clone(); + let session_name_update_tx = self.config.session_name_update_tx.clone(); + let session_id_for_title = session_id.to_string(); + let runtime = tokio::runtime::Handle::current(); + provider.set_session_title_callback(Arc::new(move |title| { + let session_manager = session_manager.clone(); + let session_name_update_tx = session_name_update_tx.clone(); + let session_id = session_id_for_title.clone(); + runtime.spawn(async move { + match session_manager + .update_name_from_provider(&session_id, title) + .await + { + Ok(Some(update)) => { + if let Some(tx) = session_name_update_tx { + let _ = tx.send(update); + } + } + Ok(None) => {} + Err(error) => warn!(%error, "Failed to apply provider session title"), + } + }); + })); // Normalize against the provider entry so custom/declarative providers // backfill `context_limit` from their known models before the config is diff --git a/crates/goose/src/session/session_manager.rs b/crates/goose/src/session/session_manager.rs index d27b8ebfd..9332e719f 100644 --- a/crates/goose/src/session/session_manager.rs +++ b/crates/goose/src/session/session_manager.rs @@ -549,6 +549,24 @@ impl SessionManager { }) } + pub async fn update_name_from_provider( + &self, + id: &str, + name: String, + ) -> Result> { + let name = name.trim().to_string(); + if name.is_empty() { + return Ok(None); + } + + let session = self.get_session(id, false).await?; + if session.user_set_name || session.name == name { + return Ok(None); + } + + Ok(Some(self.system_generated_name_update(id, name).await?)) + } + pub async fn maybe_update_name( &self, id: &str, @@ -598,7 +616,13 @@ impl SessionManager { .filter(|m| matches!(m.role, Role::User)) .count(); - if user_message_count <= MSG_COUNT_FOR_SESSION_NAME_GENERATION { + let should_generate_name = if provider.manages_own_context() { + user_message_count == 1 + } else { + user_message_count <= MSG_COUNT_FOR_SESSION_NAME_GENERATION + }; + + if should_generate_name { let name = generate_session_name(provider.as_ref(), &model_config, id, &conversation).await?; return Ok(Some(self.system_generated_name_update(id, name).await?)); @@ -2678,6 +2702,8 @@ mod tests { struct NamingTestProvider; + struct StatefulNamingTestProvider; + #[async_trait::async_trait] impl Provider for NamingTestProvider { fn get_name(&self) -> &str { @@ -2708,6 +2734,27 @@ mod tests { } } + #[async_trait::async_trait] + impl Provider for StatefulNamingTestProvider { + fn get_name(&self) -> &str { + "stateful-naming-test" + } + + async fn stream( + &self, + _model_config: &ModelConfig, + _system: &str, + _messages: &[Message], + _tools: &[Tool], + ) -> Result { + panic!("stateful session naming must not call the provider") + } + + fn manages_own_context(&self) -> bool { + true + } + } + fn naming_test_provider() -> Arc { Arc::new(NamingTestProvider) } @@ -2976,6 +3023,95 @@ mod tests { assert!(!reloaded.user_set_name); } + #[tokio::test] + async fn test_maybe_update_name_uses_local_name_for_stateful_provider() { + let temp_dir = TempDir::new().unwrap(); + let sm = SessionManager::new(temp_dir.path().to_path_buf()); + let session = sm + .create_session( + temp_dir.path().to_path_buf(), + "New Chat".to_string(), + SessionType::User, + GooseMode::default(), + ) + .await + .unwrap(); + + sm.update(&session.id) + .model_config(ModelConfig::new("test-model")) + .apply() + .await + .unwrap(); + sm.add_message( + &session.id, + &Message::user().with_text("investigate session naming with ACP providers"), + ) + .await + .unwrap(); + + let update = sm + .maybe_update_name(&session.id, Arc::new(StatefulNamingTestProvider)) + .await + .unwrap() + .unwrap(); + + assert_eq!(update.name, "investigate session naming with"); + } + + #[tokio::test] + async fn test_provider_name_replaces_generated_name_but_not_user_name() { + let temp_dir = TempDir::new().unwrap(); + let sm = SessionManager::new(temp_dir.path().to_path_buf()); + let session = sm + .create_session( + temp_dir.path().to_path_buf(), + "Local fallback".to_string(), + SessionType::User, + GooseMode::default(), + ) + .await + .unwrap(); + + let update = sm + .update_name_from_provider(&session.id, " Better ACP title ".to_string()) + .await + .unwrap() + .unwrap(); + assert_eq!(update.name, "Better ACP title"); + + sm.update(&session.id) + .model_config(ModelConfig::new("test-model")) + .apply() + .await + .unwrap(); + add_user_message(&sm, &session.id).await; + add_user_message(&sm, &session.id).await; + assert!(sm + .maybe_update_name(&session.id, Arc::new(StatefulNamingTestProvider)) + .await + .unwrap() + .is_none()); + assert_eq!( + sm.get_session(&session.id, false).await.unwrap().name, + "Better ACP title" + ); + + sm.update(&session.id) + .user_provided_name("Manual title") + .apply() + .await + .unwrap(); + assert!(sm + .update_name_from_provider(&session.id, "Another ACP title".to_string()) + .await + .unwrap() + .is_none()); + assert_eq!( + sm.get_session(&session.id, false).await.unwrap().name, + "Manual title" + ); + } + #[tokio::test] async fn test_maybe_update_name_preserves_user_renamed_session() { let temp_dir = TempDir::new().unwrap(); diff --git a/crates/goose/src/session/session_naming.rs b/crates/goose/src/session/session_naming.rs index 8391d252c..f5130bb5d 100644 --- a/crates/goose/src/session/session_naming.rs +++ b/crates/goose/src/session/session_naming.rs @@ -145,15 +145,22 @@ pub(crate) async fn generate_session_name( SESSION_NAME_SUFFIX, ); let message = Message::user().with_text(&user_text); - let result = crate::model_config::complete_fast( - provider, - model_config, - session_id, - &system, - &[message], - &[], - ) - .await?; + let result = if provider.manages_own_context() { + crate::providers::cli_common::generate_simple_session_description( + provider.get_name(), + &[message], + )? + } else { + crate::model_config::complete_fast( + provider, + model_config, + session_id, + &system, + &[message], + &[], + ) + .await? + }; let raw: String = result .0