tkmind/tkmind_go
1
0
Fork 0
Code Issues Pull Requests Actions 5 Packages Projects Releases Wiki Activity

fix: keep ACP session naming out of live conversations (#10963)

Browse Source 
Co-authored-by: Douwe M Osinga <douwe@sidewalklabs.com>
This commit is contained in:
Douwe Osinga
2026-08-05 17:36:47 +02:00
committed by GitHub
parent 1ecd746bde
commit 6646a0f6e2
5 changed files with 237 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/goose-provider-types/src/base.rs
+3
View File
@@ -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<dyn Fn(String) + Send + Sync>) {}
/// Configure OAuth authentication for this provider
///
/// This method is called when a provider has configuration keys marked with oauth_flow = true.
crates/goose/src/acp/provider.rs
+58
View File
@@ -142,6 +142,30 @@ struct HandoffContextClaim {
include_context: bool,
}
type SessionTitleCallback = Arc<dyn Fn(String) + Send + Sync>;
#[derive(Clone, Default)]
struct SessionTitlePublisher {
callback: Arc<Mutex<Option<SessionTitleCallback>>>,
}
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<Mutex<GooseMode>>,
@@ -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<AtomicU64>,
session_title_publisher: SessionTitlePublisher,
/// Config option id used to select the model, if this agent supports it.
model_config_option_id: Option<String>,
@@ -245,11 +270,13 @@ impl AcpProvider {
let pending_tool_updates: Arc<Mutex<HashMap<String, AccumulatedToolCall>>> =
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<dyn Fn(String) + Send + Sync>) {
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<Mutex<Option<mpsc::Sender<AcpUpdate>>>>,
pending_tool_updates: Arc<Mutex<HashMap<String, AccumulatedToolCall>>>,
context_size: Arc<AtomicU64>,
session_title_publisher: SessionTitlePublisher,
}
impl AcpClientLoop {
@@ -706,6 +739,7 @@ impl AcpClientLoop {
goose_mode: Arc<Mutex<GooseMode>>,
pending_tool_updates: Arc<Mutex<HashMap<String, AccumulatedToolCall>>>,
context_size: Arc<AtomicU64>,
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<mpsc::Sender<ClientRequest>>,
) -> (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,
crates/goose/src/agents/agent.rs
+23
View File
@@ -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
crates/goose/src/session/session_manager.rs
+137 -1
View File
@@ -549,6 +549,24 @@ impl SessionManager {
})
}
pub async fn update_name_from_provider(
&self,
id: &str,
name: String,
) -> Result<Option<SessionNameUpdate>> {
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<MessageStream, ProviderError> {
panic!("stateful session naming must not call the provider")
}
fn manages_own_context(&self) -> bool {
true
}
}
fn naming_test_provider() -> Arc<dyn Provider> {
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();
crates/goose/src/session/session_naming.rs
+16 -9
View File
@@ -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