diff --git a/crates/goose-cli/src/session/builder.rs b/crates/goose-cli/src/session/builder.rs index 2bbc0e70..06f81650 100644 --- a/crates/goose-cli/src/session/builder.rs +++ b/crates/goose-cli/src/session/builder.rs @@ -120,7 +120,7 @@ async fn offer_extension_debugging_help( std::env::temp_dir().join(format!("goose_debug_extension_{}.jsonl", extension_name)); // Create the debugging session - let mut debug_session = Session::new(debug_agent, temp_session_file.clone(), false, None); + let mut debug_session = Session::new(debug_agent, temp_session_file.clone(), false, None, true); // Process the debugging request println!("{}", style("Analyzing the extension failure...").yellow()); @@ -366,6 +366,7 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> Session { session_file.clone(), session_config.debug, session_config.scheduled_job_id.clone(), + !session_config.no_session, // save_session is the inverse of no_session ); // Add extensions if provided diff --git a/crates/goose-cli/src/session/mod.rs b/crates/goose-cli/src/session/mod.rs index 6f09c1ee..99ce513c 100644 --- a/crates/goose-cli/src/session/mod.rs +++ b/crates/goose-cli/src/session/mod.rs @@ -52,6 +52,7 @@ pub struct Session { debug: bool, // New field for debug mode run_mode: RunMode, scheduled_job_id: Option, // ID of the scheduled job that triggered this session + save_session: bool, // Whether to save session to file } // Cache structure for completion data @@ -113,13 +114,19 @@ impl Session { session_file: PathBuf, debug: bool, scheduled_job_id: Option, + save_session: bool, ) -> Self { - let messages = match session::read_messages(&session_file) { - Ok(msgs) => msgs, - Err(e) => { - eprintln!("Warning: Failed to load message history: {}", e); - Vec::new() + let messages = if save_session { + match session::read_messages(&session_file) { + Ok(msgs) => msgs, + Err(e) => { + eprintln!("Warning: Failed to load message history: {}", e); + Vec::new() + } } + } else { + // Don't try to read messages if we're not saving sessions + Vec::new() }; Session { @@ -130,6 +137,7 @@ impl Session { debug, run_mode: RunMode::Normal, scheduled_job_id, + save_session, } } @@ -319,6 +327,7 @@ impl Session { &self.messages, Some(provider), self.scheduled_job_id.clone(), + self.save_session, ) .await?; @@ -431,6 +440,7 @@ impl Session { &self.messages, Some(provider), self.scheduled_job_id.clone(), + self.save_session, ) .await?; @@ -619,6 +629,7 @@ impl Session { &self.messages, Some(provider), self.scheduled_job_id.clone(), + self.save_session, ) .await?; @@ -792,7 +803,7 @@ impl Session { Err(ToolError::ExecutionError("Tool call cancelled by user".to_string())) )); self.messages.push(response_message); - session::persist_messages_with_schedule_id(&self.session_file, &self.messages, None, self.scheduled_job_id.clone()).await?; + session::persist_messages_with_schedule_id(&self.session_file, &self.messages, None, self.scheduled_job_id.clone(), self.save_session).await?; drop(stream); break; @@ -889,7 +900,7 @@ impl Session { self.messages.push(message.clone()); // No need to update description on assistant messages - session::persist_messages_with_schedule_id(&self.session_file, &self.messages, None, self.scheduled_job_id.clone()).await?; + session::persist_messages_with_schedule_id(&self.session_file, &self.messages, None, self.scheduled_job_id.clone(), self.save_session).await?; if interactive {output::hide_thinking()}; let _ = progress_bars.hide(); @@ -1093,6 +1104,7 @@ impl Session { &self.messages, None, self.scheduled_job_id.clone(), + self.save_session, ) .await?; @@ -1108,6 +1120,7 @@ impl Session { &self.messages, None, self.scheduled_job_id.clone(), + self.save_session, ) .await?; @@ -1128,6 +1141,7 @@ impl Session { &self.messages, None, self.scheduled_job_id.clone(), + self.save_session, ) .await?; diff --git a/crates/goose/src/scheduler.rs b/crates/goose/src/scheduler.rs index e4f29030..8c256dcd 100644 --- a/crates/goose/src/scheduler.rs +++ b/crates/goose/src/scheduler.rs @@ -1248,6 +1248,7 @@ async fn run_scheduled_job_internal( &session_file_path, &updated_metadata, &all_session_messages, + true, ) { tracing::error!( "[Job {}] Failed to persist final messages: {}", @@ -1278,6 +1279,7 @@ async fn run_scheduled_job_internal( &session_file_path, &fallback_metadata, &all_session_messages, + true, ) { tracing::error!("[Job {}] Failed to persist final messages with fallback metadata: {}", job.id, e_fb); } @@ -1304,9 +1306,12 @@ async fn run_scheduled_job_internal( message_count: 0, ..Default::default() }; - if let Err(e) = - crate::session::storage::save_messages_with_metadata(&session_file_path, &metadata, &[]) - { + if let Err(e) = crate::session::storage::save_messages_with_metadata( + &session_file_path, + &metadata, + &[], + true, + ) { tracing::error!( "[Job {}] Failed to persist metadata for empty job: {}", job.id, diff --git a/crates/goose/src/session/storage.rs b/crates/goose/src/session/storage.rs index 4e5d1305..35c60cb3 100644 --- a/crates/goose/src/session/storage.rs +++ b/crates/goose/src/session/storage.rs @@ -1054,7 +1054,7 @@ pub async fn persist_messages( messages: &[Message], provider: Option>, ) -> Result<()> { - persist_messages_with_schedule_id(session_file, messages, provider, None).await + persist_messages_with_schedule_id(session_file, messages, provider, None, true).await } /// Write messages to a session file with metadata, including an optional scheduled job ID @@ -1071,7 +1071,13 @@ pub async fn persist_messages_with_schedule_id( messages: &[Message], provider: Option>, schedule_id: Option, + save_session: bool, ) -> Result<()> { + if !save_session { + tracing::debug!("Skipping session persistence (save_session=false)"); + return Ok(()); + } + // Validate the session file path for security let secure_path = get_path(Identifier::Path(session_file.to_path_buf()))?; @@ -1091,8 +1097,14 @@ pub async fn persist_messages_with_schedule_id( match provider { Some(provider) if user_message_count < 4 => { //generate_description is responsible for writing the messages - generate_description_with_schedule_id(&secure_path, messages, provider, schedule_id) - .await + generate_description_with_schedule_id( + &secure_path, + messages, + provider, + schedule_id, + save_session, + ) + .await } _ => { // Read existing metadata @@ -1102,7 +1114,7 @@ pub async fn persist_messages_with_schedule_id( metadata.schedule_id = schedule_id; } // Write the file with metadata and messages - save_messages_with_metadata(&secure_path, &metadata, messages) + save_messages_with_metadata(&secure_path, &metadata, messages, save_session) } } } @@ -1124,7 +1136,13 @@ pub fn save_messages_with_metadata( session_file: &Path, metadata: &SessionMetadata, messages: &[Message], + save_session: bool, ) -> Result<()> { + if !save_session { + tracing::debug!("Skipping session file write (save_session=false)"); + return Ok(()); + } + use fs2::FileExt; // Validate the path for security @@ -1239,7 +1257,7 @@ pub async fn generate_description( messages: &[Message], provider: Arc, ) -> Result<()> { - generate_description_with_schedule_id(session_file, messages, provider, None).await + generate_description_with_schedule_id(session_file, messages, provider, None, true).await } /// Generate a description for the session using the provider, including an optional scheduled job ID @@ -1256,7 +1274,13 @@ pub async fn generate_description_with_schedule_id( messages: &[Message], provider: Arc, schedule_id: Option, + save_session: bool, ) -> Result<()> { + if !save_session { + tracing::debug!("Skipping description generation (save_session=false)"); + return Ok(()); + } + // Validate the path for security let secure_path = get_path(Identifier::Path(session_file.to_path_buf()))?; @@ -1331,7 +1355,7 @@ pub async fn generate_description_with_schedule_id( } // Update the file with the new metadata and existing messages - save_messages_with_metadata(&secure_path, &metadata, messages) + save_messages_with_metadata(&secure_path, &metadata, messages, save_session) } /// Update only the metadata in a session file, preserving all messages @@ -1347,7 +1371,7 @@ pub async fn update_metadata(session_file: &Path, metadata: &SessionMetadata) -> let messages = read_messages(&secure_path)?; // Rewrite the file with the new metadata and existing messages - save_messages_with_metadata(&secure_path, metadata, &messages) + save_messages_with_metadata(&secure_path, metadata, &messages, true) } #[cfg(test)] @@ -1662,7 +1686,7 @@ mod tests { let messages = vec![Message::user().with_text("test")]; // Write with special metadata - save_messages_with_metadata(&file_path, &metadata, &messages)?; + save_messages_with_metadata(&file_path, &metadata, &messages, true)?; // Read back metadata let read_metadata = read_metadata(&file_path)?; @@ -1686,7 +1710,7 @@ mod tests { // Test deserialization of invalid directory let messages = vec![Message::user().with_text("test")]; - save_messages_with_metadata(&file_path, &metadata, &messages)?; + save_messages_with_metadata(&file_path, &metadata, &messages, true)?; // Modify the file to include invalid directory let contents = fs::read_to_string(&file_path)?; @@ -1755,4 +1779,84 @@ mod tests { let normalized_existing = normalize_path_for_comparison(&test_path); assert!(!normalized_existing.as_os_str().is_empty()); } + + #[tokio::test] + async fn test_save_session_parameter() -> Result<()> { + let dir = tempdir()?; + let file_path = dir.path().join("test_save_session.jsonl"); + + let messages = vec![ + Message::user().with_text("Hello"), + Message::assistant().with_text("Hi there"), + ]; + + let metadata = SessionMetadata::default(); + + // Test with save_session = false - should not create file + save_messages_with_metadata(&file_path, &metadata, &messages, false)?; + assert!( + !file_path.exists(), + "File should not be created when save_session=false" + ); + + // Test with save_session = true - should create file + save_messages_with_metadata(&file_path, &metadata, &messages, true)?; + assert!( + file_path.exists(), + "File should be created when save_session=true" + ); + + // Verify content is correct + let read_messages = read_messages(&file_path)?; + assert_eq!(messages.len(), read_messages.len()); + + Ok(()) + } + + #[tokio::test] + async fn test_persist_messages_with_save_session_false() -> Result<()> { + let dir = tempdir()?; + let file_path = dir.path().join("test_persist_no_save.jsonl"); + + let messages = vec![ + Message::user().with_text("Test message"), + Message::assistant().with_text("Test response"), + ]; + + // Test persist_messages_with_schedule_id with save_session = false + persist_messages_with_schedule_id( + &file_path, + &messages, + None, + Some("test_schedule".to_string()), + false, + ) + .await?; + + assert!( + !file_path.exists(), + "File should not be created when save_session=false" + ); + + // Test persist_messages_with_schedule_id with save_session = true + persist_messages_with_schedule_id( + &file_path, + &messages, + None, + Some("test_schedule".to_string()), + true, + ) + .await?; + + assert!( + file_path.exists(), + "File should be created when save_session=true" + ); + + // Verify the schedule_id was set correctly + let metadata = read_metadata(&file_path)?; + assert_eq!(metadata.schedule_id, Some("test_schedule".to_string())); + + Ok(()) + } } diff --git a/crates/goose/tests/private_tests.rs b/crates/goose/tests/private_tests.rs index 78e7cdd7..af05a9b4 100644 --- a/crates/goose/tests/private_tests.rs +++ b/crates/goose/tests/private_tests.rs @@ -788,7 +788,7 @@ async fn test_schedule_tool_session_content_action_with_real_session() { ]; // Save the session file - goose::session::storage::save_messages_with_metadata(&session_path, &metadata, &messages) + goose::session::storage::save_messages_with_metadata(&session_path, &metadata, &messages, true) .unwrap(); // Test the session_content action