diff --git a/crates/goose-cli/src/cli.rs b/crates/goose-cli/src/cli.rs index 4b2e5e0144fe..76094eedde8c 100644 --- a/crates/goose-cli/src/cli.rs +++ b/crates/goose-cli/src/cli.rs @@ -67,17 +67,27 @@ jsonl' -> '20250325_200615')." path: Option, } -async fn get_session_id(identifier: Identifier) -> Result { +async fn get_session_id(identifier: Identifier, must_exist: bool) -> Result { if let Some(session_id) = identifier.session_id { Ok(session_id) } else if let Some(name) = identifier.name { let sessions = SessionManager::list_sessions().await?; - sessions + match sessions .into_iter() .find(|s| s.id == name || s.description.contains(&name)) .map(|s| s.id) - .ok_or_else(|| anyhow::anyhow!("No session found with name '{}'", name)) + { + Some(id) => Ok(id), + None => { + if must_exist { + Err(anyhow::anyhow!("No session found with name '{}'", name)) + } else { + // Return the name as-is to be used as a new session ID/description + Ok(name) + } + } + } } else if let Some(path) = identifier.path { path.file_stem() .and_then(|s| s.to_str()) @@ -826,7 +836,7 @@ pub async fn cli() -> Result<()> { format, }) => { let session_identifier = if let Some(id) = identifier { - get_session_id(id).await? + get_session_id(id, true).await? } else { // If no identifier is provided, prompt for interactive selection match crate::commands::session::prompt_interactive_session_selection().await @@ -859,7 +869,7 @@ pub async fn cli() -> Result<()> { ); let session_id = if let Some(id) = identifier { - Some(get_session_id(id).await?) + Some(get_session_id(id, resume).await?) } else { None }; @@ -1057,7 +1067,7 @@ pub async fn cli() -> Result<()> { } }; let session_id = if let Some(id) = identifier { - Some(get_session_id(id).await?) + Some(get_session_id(id, resume).await?) } else { None }; @@ -1281,3 +1291,176 @@ pub async fn cli() -> Result<()> { } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn test_get_session_id_with_direct_session_id() { + // Test that session_id field is returned as-is + let identifier = Identifier { + name: None, + session_id: Some("test_session_123".to_string()), + path: None, + }; + + let result = get_session_id(identifier, false).await; + assert!(result.is_ok()); + assert_eq!(result.unwrap(), "test_session_123"); + } + + #[tokio::test] + async fn test_get_session_id_with_path() { + // Test that session ID is extracted from path + let identifier = Identifier { + name: None, + session_id: None, + path: Some(PathBuf::from("/some/path/20250107_001.jsonl")), + }; + + let result = get_session_id(identifier, false).await; + assert!(result.is_ok()); + assert_eq!(result.unwrap(), "20250107_001"); + } + + #[tokio::test] + async fn test_get_session_id_must_exist_false_with_nonexistent_name() { + // Test that when must_exist=false, non-existent session names are returned as-is + let identifier = Identifier { + name: Some("nonexistent_session_xyz".to_string()), + session_id: None, + path: None, + }; + + let result = get_session_id(identifier, false).await; + assert!(result.is_ok()); + // Should return the name as-is since it doesn't exist but must_exist=false + assert_eq!(result.unwrap(), "nonexistent_session_xyz"); + } + + #[tokio::test] + async fn test_get_session_id_must_exist_true_with_nonexistent_name() { + // Test that when must_exist=true, non-existent session names cause an error + let identifier = Identifier { + name: Some("definitely_nonexistent_session_999".to_string()), + session_id: None, + path: None, + }; + + let result = get_session_id(identifier, true).await; + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("No session found with name")); + } + + #[tokio::test] + async fn test_get_session_id_must_exist_true_with_existing_session() { + // Test that when must_exist=true and session exists, it returns the session ID + + // Create a test session + let session = match SessionManager::create_session( + std::env::current_dir().unwrap(), + "Test session for get_session_id".to_string(), + ) + .await + { + Ok(s) => s, + Err(_) => { + // Skip test if we can't create a session (e.g., in CI without proper setup) + return; + } + }; + + let session_id = session.id.clone(); + + // Test with the actual session ID + let identifier = Identifier { + name: Some(session_id.clone()), + session_id: None, + path: None, + }; + + let result = get_session_id(identifier, true).await; + if result.is_ok() { + assert_eq!(result.unwrap(), session_id); + } + + // Clean up + let _ = SessionManager::delete_session(&session_id).await; + } + + #[tokio::test] + async fn test_get_session_id_finds_by_description() { + // Test that get_session_id can find sessions by description + + // Create a test session with a specific description + let test_description = "Unique test description 12345"; + let session = match SessionManager::create_session( + std::env::current_dir().unwrap(), + test_description.to_string(), + ) + .await + { + Ok(s) => s, + Err(_) => { + // Skip test if we can't create a session + return; + } + }; + + let session_id = session.id.clone(); + + // Test searching by part of the description + let identifier = Identifier { + name: Some("Unique test description".to_string()), + session_id: None, + path: None, + }; + + let result = get_session_id(identifier, true).await; + if result.is_ok() { + assert_eq!(result.unwrap(), session_id); + } + + // Clean up + let _ = SessionManager::delete_session(&session_id).await; + } + + #[tokio::test] + async fn test_get_session_id_must_exist_false_with_existing_session() { + // Test that when must_exist=false and session exists, it still returns the session ID + + // Create a test session + let session = match SessionManager::create_session( + std::env::current_dir().unwrap(), + "Test existing session".to_string(), + ) + .await + { + Ok(s) => s, + Err(_) => { + // Skip test if we can't create a session + return; + } + }; + + let session_id = session.id.clone(); + + let identifier = Identifier { + name: Some(session_id.clone()), + session_id: None, + path: None, + }; + + let result = get_session_id(identifier, false).await; + if result.is_ok() { + assert_eq!(result.unwrap(), session_id); + } + + // Clean up + let _ = SessionManager::delete_session(&session_id).await; + } +} diff --git a/crates/goose-cli/src/session/builder.rs b/crates/goose-cli/src/session/builder.rs index f5dde78bcecc..dc25e18cfd23 100644 --- a/crates/goose-cli/src/session/builder.rs +++ b/crates/goose-cli/src/session/builder.rs @@ -309,7 +309,26 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> CliSession { } } } else if let Some(session_id) = session_config.session_id { - Some(session_id) + // Check if the session exists; if not, we'll create a new one with this ID later + match SessionManager::get_session(&session_id, false).await { + Ok(_) => { + // Session exists, resume it (for backward compatibility) + Some(session_id) + } + Err(_) => { + // Session doesn't exist, create a new one with this ID + let session = SessionManager::create_session( + std::env::current_dir().unwrap(), + session_id.clone(), + ) + .await + .unwrap_or_else(|e| { + output::render_error(&format!("Failed to create session: {}", e)); + process::exit(1); + }); + Some(session.id) + } + } } else { let session = SessionManager::create_session( std::env::current_dir().unwrap(), diff --git a/crates/goose-cli/src/session/mod.rs b/crates/goose-cli/src/session/mod.rs index cb80d7381fd5..603fe118ba02 100644 --- a/crates/goose-cli/src/session/mod.rs +++ b/crates/goose-cli/src/session/mod.rs @@ -130,10 +130,13 @@ impl CliSession { let messages = if let Some(session_id) = &session_id { tokio::task::block_in_place(|| { tokio::runtime::Handle::current().block_on(async { - SessionManager::get_session(session_id, true) - .await - .map(|session| session.conversation.unwrap_or_default()) - .unwrap() + match SessionManager::get_session(session_id, true).await { + Ok(session) => session.conversation.unwrap_or_default(), + Err(e) => { + tracing::warn!("Failed to load session '{}': {}. Starting with empty conversation.", session_id, e); + Conversation::new_unvalidated(Vec::new()) + } + } }) }) } else { @@ -1748,4 +1751,101 @@ mod tests { let duration = Duration::from_millis(60500); assert_eq!(format_elapsed_time(duration), "1m 00s"); } + + #[tokio::test(flavor = "multi_thread")] + async fn test_cli_session_new_with_nonexistent_session() { + // Test that CliSession::new handles non-existent sessions gracefully + // without panicking, even when a session_id is provided + + let agent = Agent::new(); + let fake_session_id = Some("nonexistent_session_12345".to_string()); + + // This should not panic - it should create a session with empty messages + let session = CliSession::new( + agent, + fake_session_id.clone(), + false, + None, + None, + None, + None, + ); + + // Verify the session was created with the provided ID + assert_eq!(session.session_id, fake_session_id); + + // Verify messages are empty (not loaded from non-existent session) + assert_eq!(session.messages.len(), 0); + } + + #[test] + fn test_cli_session_new_without_session_id() { + // Test that CliSession::new works correctly when no session_id is provided + + let agent = Agent::new(); + + let session = CliSession::new( + agent, + None, + false, + None, + None, + None, + None, + ); + + // Verify no session ID is set + assert_eq!(session.session_id, None); + + // Verify messages are empty + assert_eq!(session.messages.len(), 0); + } + + #[tokio::test(flavor = "multi_thread")] + async fn test_cli_session_new_with_existing_session() { + // Test that CliSession::new loads messages from an existing session + + // First, create a session in the database + let session = match SessionManager::create_session( + std::env::current_dir().unwrap(), + "Test session for loading".to_string(), + ) + .await + { + Ok(s) => s, + Err(_) => { + // Skip test if we can't create a session + return; + } + }; + + let session_id = session.id.clone(); + + // Add a test message to the session + let test_message = Message::user().with_text("Test message"); + if let Err(_) = SessionManager::add_message(&session_id, &test_message).await { + // Clean up and skip if we can't add message + let _ = SessionManager::delete_session(&session_id).await; + return; + } + + // Now create a CliSession with this session ID + let agent = Agent::new(); + let cli_session = CliSession::new( + agent, + Some(session_id.clone()), + false, + None, + None, + None, + None, + ); + + // Verify the session was loaded with messages + assert_eq!(cli_session.session_id, Some(session_id.clone())); + assert_eq!(cli_session.messages.len(), 1); + + // Clean up: delete the test session + let _ = SessionManager::delete_session(&session_id).await; + } }