feat(acp): title new sessions from _meta.sessionTitle (#10712)
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
@@ -19,7 +19,16 @@ struct InitialSessionConfig {
|
||||
extension_data: ExtensionData,
|
||||
recipe: Option<Recipe>,
|
||||
user_recipe_values: Option<HashMap<String, String>>,
|
||||
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<String>,
|
||||
/// 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<String>,
|
||||
}
|
||||
|
||||
impl GooseAcpAgent {
|
||||
@@ -30,14 +39,14 @@ impl GooseAcpAgent {
|
||||
) -> Result<NewSessionResponse, agent_client_protocol::Error> {
|
||||
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<String>,
|
||||
meta: NewSessionMetaFields,
|
||||
) -> Result<NewSessionResponse, agent_client_protocol::Error> {
|
||||
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<String>,
|
||||
meta: NewSessionMetaFields,
|
||||
) -> Result<Option<Recipe>, 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<bool, agent_client_protoc
|
||||
})
|
||||
}
|
||||
|
||||
fn recipe_title(recipe: Option<&(Recipe, PathBuf)>) -> 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<NewSessionMetaFields, agent_client_protocol::Error> {
|
||||
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<Option<Vec<GooseExtension>>, agent_client_protocol::Error> {
|
||||
|
||||
@@ -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<turn-context>"#;
|
||||
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<turn-context>"#;
|
||||
/// 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<C: Connection>() {
|
||||
_ => 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));
|
||||
|
||||
@@ -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<String, serde_json::Value> {
|
||||
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<String, serde_json::Value>,
|
||||
) -> anyhow::Result<String> {
|
||||
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<String, serde_json::Value> {
|
||||
@@ -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, serde_json::Value>,
|
||||
) -> (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,
|
||||
),
|
||||
],
|
||||
<AcpServerConnection as Connection>::expected_session_id(),
|
||||
)
|
||||
.await;
|
||||
let conn = <AcpServerConnection as Connection>::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::<AcpServerConnection>().await });
|
||||
|
||||
Reference in New Issue
Block a user