From 85d14b74698e4af3ac63c13833dd54a53abfa106 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Sun, 26 Jul 2026 19:40:59 -0400 Subject: [PATCH] feat(acp): title new sessions from _meta.sessionTitle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ACP clients that manage many concurrent sessions have no way to label them: goose names a session from its recipe title or falls back to "New Chat", so every client-created session looks alike in session lists. Read a harness-neutral `_meta.sessionTitle` on `session/new` and use it as the session name. A recipe title still wins — it is a server-side declaration, and deferring to it preserves today's behavior exactly. The client title is recorded via `user_provided_name`, which marks the session user-set so `maybe_update_name` does not replace it with a generated one. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/goose/src/acp/server/new_session.rs | 56 ++++- crates/goose/tests/acp_common_tests/mod.rs | 8 +- crates/goose/tests/acp_server_test.rs | 247 ++++++++++++++++++++- 3 files changed, 293 insertions(+), 18 deletions(-) diff --git a/crates/goose/src/acp/server/new_session.rs b/crates/goose/src/acp/server/new_session.rs index b3c192dc11c9..db05ce73bca4 100644 --- a/crates/goose/src/acp/server/new_session.rs +++ b/crates/goose/src/acp/server/new_session.rs @@ -19,7 +19,16 @@ struct InitialSessionConfig { extension_data: ExtensionData, recipe: Option, user_recipe_values: Option>, + meta: NewSessionMetaFields, +} + +/// Session fields read from `_meta` on `session/new` that are applied to the +/// session row after it is created. +struct NewSessionMetaFields { project_id: Option, + /// Client-supplied title, recorded as user-set so goose's own name + /// generation leaves it alone. `None` when a recipe title took precedence. + client_title: Option, } impl GooseAcpAgent { @@ -30,14 +39,14 @@ impl GooseAcpAgent { ) -> Result { validate_absolute_cwd(&args.cwd)?; let config = Config::global(); - let project_id = meta_string(args.meta.as_ref(), "projectId")?; let session_type = session_type_from_meta(args.meta.as_ref())?; let current_mode: GooseMode = config.get_goose_mode().unwrap_or_default(); let recipe = self.resolve_recipe_from_meta(args.meta.as_ref()).await?; - let session_name = match recipe.as_ref() { - Some((recipe, _)) if !recipe.title.trim().is_empty() => recipe.title.clone(), - _ => "New Chat".to_string(), - }; + let meta = new_session_meta_fields(args.meta.as_ref(), recipe.as_ref())?; + let session_name = recipe_title(recipe.as_ref()) + .map(str::to_string) + .or_else(|| meta.client_title.clone()) + .unwrap_or_else(|| "New Chat".to_string()); let session = self .session_manager @@ -45,7 +54,7 @@ impl GooseAcpAgent { .await .internal_err_ctx("Failed to create session")?; match self - .finish_new_session_setup(cx, config, &session, args, recipe, project_id) + .finish_new_session_setup(cx, config, &session, args, recipe, meta) .await { Ok(response) => Ok(response), @@ -63,10 +72,10 @@ impl GooseAcpAgent { session: &Session, args: NewSessionRequest, recipe: Option<(Recipe, PathBuf)>, - project_id: Option, + meta: NewSessionMetaFields, ) -> Result { let rendered_recipe = self - .configure_new_session(cx, config, session, args, recipe, project_id) + .configure_new_session(cx, config, session, args, recipe, meta) .await?; let reloaded_session = self.reload_session(&session.id).await?; @@ -111,7 +120,7 @@ impl GooseAcpAgent { session: &Session, args: NewSessionRequest, recipe: Option<(Recipe, PathBuf)>, - project_id: Option, + meta: NewSessionMetaFields, ) -> Result, agent_client_protocol::Error> { let (rendered, user_recipe_values) = self .render_recipe_for_session(cx, &session.id, recipe.as_ref()) @@ -140,7 +149,7 @@ impl GooseAcpAgent { extension_data, recipe: recipe.map(|(recipe, _)| recipe), user_recipe_values, - project_id, + meta, }, ) .await?; @@ -211,9 +220,12 @@ impl GooseAcpAgent { if config.user_recipe_values.is_some() { builder = builder.user_recipe_values(config.user_recipe_values); } - if let Some(project_id) = config.project_id { + if let Some(project_id) = config.meta.project_id { builder = builder.project_id(Some(project_id)); } + if let Some(client_title) = config.meta.client_title { + builder = builder.user_provided_name(client_title); + } builder .apply() .await @@ -271,6 +283,28 @@ fn meta_bool(meta: Option<&Meta>, key: &str) -> Result) -> Option<&str> { + recipe + .map(|(recipe, _)| recipe.title.trim()) + .filter(|title| !title.is_empty()) +} + +fn new_session_meta_fields( + meta: Option<&Meta>, + recipe: Option<&(Recipe, PathBuf)>, +) -> Result { + let session_title = meta_string(meta, "sessionTitle")? + .map(|title| title.trim().to_string()) + .filter(|title| !title.is_empty()); + Ok(NewSessionMetaFields { + project_id: meta_string(meta, "projectId")?, + // A recipe title is a server-side declaration, so it keeps the + // precedence it has today and a client title only replaces the + // "New Chat" fallback. + client_title: session_title.filter(|_| recipe_title(recipe).is_none()), + }) +} + fn meta_goose_extensions( meta: Option<&Meta>, ) -> Result>, agent_client_protocol::Error> { diff --git a/crates/goose/tests/acp_common_tests/mod.rs b/crates/goose/tests/acp_common_tests/mod.rs index 7c3d2185ee93..e7d084dae4b1 100644 --- a/crates/goose/tests/acp_common_tests/mod.rs +++ b/crates/goose/tests/acp_common_tests/mod.rs @@ -22,8 +22,10 @@ use std::sync::Arc; use std::time::Duration; const SHELL_TEST_CONTENT: &str = "test-shell-content-98765"; -const TURN_CONTEXT_OPEN: &str = r#"\n"#; -const OPENAI_SESSION_NAME_RESPONSE: &str = r#"data: {"id":"chatcmpl-test","object":"chat.completion.chunk","created":1766229303,"model":"gpt-5-nano","choices":[{"index":0,"delta":{"role":"assistant","content":""},"finish_reason":null}]} +pub const TURN_CONTEXT_OPEN: &str = r#"\n"#; +/// Session name produced by `OPENAI_SESSION_NAME_RESPONSE`. +pub const GENERATED_SESSION_TITLE: &str = "Generated Test Title"; +pub const OPENAI_SESSION_NAME_RESPONSE: &str = r#"data: {"id":"chatcmpl-test","object":"chat.completion.chunk","created":1766229303,"model":"gpt-5-nano","choices":[{"index":0,"delta":{"role":"assistant","content":""},"finish_reason":null}]} data: {"id":"chatcmpl-test","object":"chat.completion.chunk","created":1766229303,"model":"gpt-5-nano","choices":[{"index":0,"delta":{"content":"Generated Test Title"},"finish_reason":null}]} @@ -157,7 +159,7 @@ pub async fn run_session_name_update_notification() { _ => None, }) .expect("expected generated session name notification"); - assert_eq!(update.0.as_deref(), Some("Generated Test Title")); + assert_eq!(update.0.as_deref(), Some(GENERATED_SESSION_TITLE)); assert!(update.1.is_some()); assert!(update.2.unwrap_or_default() >= 1); assert_eq!(*update.3, Some(false)); diff --git a/crates/goose/tests/acp_server_test.rs b/crates/goose/tests/acp_server_test.rs index a75b348f1969..7501216e4c64 100644 --- a/crates/goose/tests/acp_server_test.rs +++ b/crates/goose/tests/acp_server_test.rs @@ -3,9 +3,9 @@ #[path = "acp_common_tests/mod.rs"] mod common_tests; use agent_client_protocol::schema::v1::{ - ListSessionsRequest, ListSessionsResponse, NewSessionRequest, SessionConfigKind, - SessionConfigOptionCategory, SessionConfigOptionValue, SessionInfo, - SetSessionConfigOptionRequest, + ContentBlock, ListSessionsRequest, ListSessionsResponse, NewSessionRequest, PromptRequest, + SessionConfigKind, SessionConfigOptionCategory, SessionConfigOptionValue, SessionInfo, + SetSessionConfigOptionRequest, StopReason, TextContent, }; use agent_client_protocol::ErrorCode; use common_tests::fixtures::server::{ @@ -26,7 +26,8 @@ use common_tests::{ run_new_session_uses_current_config_mode, run_permission_persistence, run_prompt_basic, run_prompt_error, run_prompt_image, run_prompt_image_attachment, run_prompt_mcp, run_prompt_model_mismatch, run_prompt_skill, run_session_name_update_notification, - run_shell_terminal_false, run_shell_terminal_true, + run_shell_terminal_false, run_shell_terminal_true, GENERATED_SESSION_TITLE, + OPENAI_SESSION_NAME_RESPONSE, TURN_CONTEXT_OPEN, }; use goose::config::GooseMode; use goose::conversation::message::{Message, MessageMetadata}; @@ -124,6 +125,51 @@ fn assert_invalid_params(error: anyhow::Error) { assert_eq!(acp_error.code, ErrorCode::InvalidParams); } +fn session_title_meta(value: serde_json::Value) -> serde_json::Map { + let mut meta = serde_json::Map::new(); + meta.insert("sessionTitle".to_string(), value); + meta +} + +async fn new_session_with_meta( + conn: &AcpServerConnection, + work_dir: &Path, + meta: serde_json::Map, +) -> anyhow::Result { + let response = conn + .cx() + .send_request(NewSessionRequest::new(work_dir).meta(meta)) + .block_task() + .await?; + Ok(response.session_id.0.to_string()) +} + +/// Returns the session's title and whether it is recorded as user-set. +async fn session_title(conn: &AcpServerConnection, session_id: &str) -> (String, bool) { + let response = get_session_info_request( + conn, + GetSessionInfoRequest { + session_id: session_id.to_string(), + }, + ) + .await + .unwrap(); + let user_set_name = response + .session + .meta + .as_ref() + .and_then(|meta| meta.get("userSetName")) + .and_then(serde_json::Value::as_bool) + .expect("session info should include userSetName"); + ( + response + .session + .title + .expect("session info should include a title"), + user_set_name, + ) +} + fn include_last_message_snippet_meta( value: serde_json::Value, ) -> serde_json::Map { @@ -752,6 +798,199 @@ fn test_new_session_cleans_up_when_config_fails() { }); } +#[test] +fn test_new_session_titles_session_from_meta_session_title() { + run_test(async { + let data_root = tempfile::tempdir().unwrap(); + let conn = new_connection(data_root.path()).await; + let work_dir = tempfile::tempdir().unwrap(); + + let session_id = new_session_with_meta( + &conn, + work_dir.path(), + session_title_meta(serde_json::json!(" Duncan in #general ")), + ) + .await + .unwrap(); + + assert_eq!( + session_title(&conn, &session_id).await, + ("Duncan in #general".to_string(), true) + ); + }); +} + +#[test] +fn test_new_session_prefers_recipe_title_over_meta_session_title() { + run_test(async { + let data_root = tempfile::tempdir().unwrap(); + let conn = new_connection(data_root.path()).await; + let work_dir = tempfile::tempdir().unwrap(); + let recipe = Recipe::builder() + .title("Recipe title") + .description("A recipe with a title") + .instructions("Follow the recipe") + .build() + .unwrap(); + let mut meta = session_title_meta(serde_json::json!("Client title")); + meta.insert( + "recipeDeeplink".to_string(), + serde_json::Value::String(recipe_deeplink::encode(&recipe).unwrap()), + ); + + let session_id = new_session_with_meta(&conn, work_dir.path(), meta) + .await + .unwrap(); + + // The recipe title wins, and the session is not marked user-set so + // goose's own recipe-title naming path still applies. + assert_eq!( + session_title(&conn, &session_id).await, + ("Recipe title".to_string(), false) + ); + }); +} + +#[test] +fn test_new_session_without_meta_session_title_uses_default_name() { + run_test(async { + let data_root = tempfile::tempdir().unwrap(); + let conn = new_connection(data_root.path()).await; + let work_dir = tempfile::tempdir().unwrap(); + + for meta in [ + serde_json::Map::new(), + session_title_meta(serde_json::Value::Null), + session_title_meta(serde_json::json!(" ")), + ] { + let session_id = new_session_with_meta(&conn, work_dir.path(), meta) + .await + .unwrap(); + + assert_eq!( + session_title(&conn, &session_id).await, + ("New Chat".to_string(), false) + ); + } + }); +} + +#[test] +fn test_new_session_rejects_non_string_meta_session_title() { + run_test(async { + let data_root = tempfile::tempdir().unwrap(); + let conn = new_connection(data_root.path()).await; + let work_dir = tempfile::tempdir().unwrap(); + let recipe = Recipe::builder() + .title("Recipe title") + .description("A recipe with a title") + .instructions("Follow the recipe") + .build() + .unwrap(); + + // Rejected on its own, and also when a recipe title would have won — + // validation does not depend on precedence. + let mut with_recipe = session_title_meta(serde_json::json!(42)); + with_recipe.insert( + "recipeDeeplink".to_string(), + serde_json::Value::String(recipe_deeplink::encode(&recipe).unwrap()), + ); + for meta in [session_title_meta(serde_json::json!(42)), with_recipe] { + let error = new_session_with_meta(&conn, work_dir.path(), meta) + .await + .unwrap_err(); + assert_invalid_params(error); + } + + let sessions = SessionManager::new(data_root.path().to_path_buf()) + .list_all_sessions() + .await + .unwrap(); + assert!(sessions.is_empty()); + }); +} + +/// Drives one naming-enabled turn and returns the session's title once name +/// generation has had a chance to run. +async fn title_after_naming_turn( + data_root: &Path, + meta: serde_json::Map, +) -> (String, bool) { + let openai = OpenAiFixture::new( + vec![ + ( + format!("what is 1+1{TURN_CONTEXT_OPEN}"), + include_str!("acp_test_data/openai_basic.txt"), + ), + ( + "Generate a short title for the above messages.".to_string(), + OPENAI_SESSION_NAME_RESPONSE, + ), + ], + ::expected_session_id(), + ) + .await; + let conn = ::new( + TestConnectionConfig { + data_root: data_root.to_path_buf(), + disable_session_naming: false, + ..Default::default() + }, + openai, + ) + .await; + let work_dir = tempfile::tempdir().unwrap(); + let session_id = new_session_with_meta(&conn, work_dir.path(), meta) + .await + .unwrap(); + + let response = conn + .cx() + .send_request(PromptRequest::new( + agent_client_protocol::schema::v1::SessionId::new(session_id.clone()), + vec![ContentBlock::Text(TextContent::new("what is 1+1"))], + )) + .block_task() + .await + .unwrap(); + assert_eq!(response.stop_reason, StopReason::EndTurn); + + // Naming runs in a spawned task: wait for it to land, or for the deadline + // to prove it never will. + let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(2); + loop { + let title = session_title(&conn, &session_id).await; + if title.0 == GENERATED_SESSION_TITLE || tokio::time::Instant::now() >= deadline { + return title; + } + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } +} + +#[test] +fn test_generated_name_does_not_replace_meta_session_title() { + run_test(async { + // Control arm: with no client title, generation names the session. + let data_root = tempfile::tempdir().unwrap(); + assert_eq!( + title_after_naming_turn(data_root.path(), serde_json::Map::new()).await, + (GENERATED_SESSION_TITLE.to_string(), false), + "name generation must work here, or the assertion below proves nothing" + ); + + // A client title is recorded as user-set, which generation must respect. + let data_root = tempfile::tempdir().unwrap(); + assert_eq!( + title_after_naming_turn( + data_root.path(), + session_title_meta(serde_json::json!("Client title")) + ) + .await, + ("Client title".to_string(), true) + ); + }); +} + #[test] fn test_model_set() { run_test(async { run_model_set::().await });