diff --git a/crates/goose-cli/src/commands/web.rs b/crates/goose-cli/src/commands/web.rs index 72b5d32b..e61975ad 100644 --- a/crates/goose-cli/src/commands/web.rs +++ b/crates/goose-cli/src/commands/web.rs @@ -475,7 +475,14 @@ async fn process_message_streaming( } let provider = provider.unwrap(); - session::persist_messages(&session_file, &messages, Some(provider.clone())).await?; + let working_dir = Some(std::env::current_dir()?); + session::persist_messages( + &session_file, + &messages, + Some(provider.clone()), + working_dir.clone(), + ) + .await?; // Create a session config let session_config = SessionConfig { @@ -503,7 +510,13 @@ async fn process_message_streaming( let session_msgs = session_messages.lock().await; session_msgs.clone() }; - session::persist_messages(&session_file, ¤t_messages, None).await?; + session::persist_messages( + &session_file, + ¤t_messages, + None, + working_dir.clone(), + ) + .await?; // Handle different message content types for content in &message.content { match content { diff --git a/crates/goose-cli/src/session/mod.rs b/crates/goose-cli/src/session/mod.rs index a0616535..5b51bd70 100644 --- a/crates/goose-cli/src/session/mod.rs +++ b/crates/goose-cli/src/session/mod.rs @@ -370,11 +370,16 @@ impl Session { // Persist messages with provider for automatic description generation if let Some(session_file) = &self.session_file { + let working_dir = Some( + std::env::current_dir().expect("failed to get current session working directory"), + ); + session::persist_messages_with_schedule_id( session_file, &self.messages, Some(provider), self.scheduled_job_id.clone(), + working_dir, ) .await?; } @@ -492,11 +497,17 @@ impl Session { // Persist messages with provider for automatic description generation if let Some(session_file) = &self.session_file { + let working_dir = Some( + std::env::current_dir() + .expect("failed to get current session working directory"), + ); + session::persist_messages_with_schedule_id( session_file, &self.messages, Some(provider), self.scheduled_job_id.clone(), + working_dir, ) .await?; } @@ -708,11 +719,13 @@ impl Session { // Persist the summarized messages if let Some(session_file) = &self.session_file { + let working_dir = std::env::current_dir().ok(); session::persist_messages_with_schedule_id( session_file, &self.messages, Some(provider), self.scheduled_job_id.clone(), + working_dir, ) .await?; } @@ -892,11 +905,13 @@ impl Session { )); push_message(&mut self.messages, response_message); if let Some(session_file) = &self.session_file { + let working_dir = std::env::current_dir().ok(); session::persist_messages_with_schedule_id( session_file, &self.messages, None, self.scheduled_job_id.clone(), + working_dir, ) .await?; } @@ -991,11 +1006,13 @@ impl Session { // No need to update description on assistant messages if let Some(session_file) = &self.session_file { + let working_dir = std::env::current_dir().ok(); session::persist_messages_with_schedule_id( session_file, &self.messages, None, self.scheduled_job_id.clone(), + working_dir, ) .await?; } @@ -1208,11 +1225,13 @@ impl Session { // No need for description update here if let Some(session_file) = &self.session_file { + let working_dir = std::env::current_dir().ok(); session::persist_messages_with_schedule_id( session_file, &self.messages, None, self.scheduled_job_id.clone(), + working_dir, ) .await?; } @@ -1225,11 +1244,13 @@ impl Session { // No need for description update here if let Some(session_file) = &self.session_file { + let working_dir = std::env::current_dir().ok(); session::persist_messages_with_schedule_id( session_file, &self.messages, None, self.scheduled_job_id.clone(), + working_dir, ) .await?; } @@ -1247,11 +1268,13 @@ impl Session { // No need for description update here if let Some(session_file) = &self.session_file { + let working_dir = std::env::current_dir().ok(); session::persist_messages_with_schedule_id( session_file, &self.messages, None, self.scheduled_job_id.clone(), + working_dir, ) .await?; } diff --git a/crates/goose-server/src/routes/reply.rs b/crates/goose-server/src/routes/reply.rs index 55d04d95..1f7f0588 100644 --- a/crates/goose-server/src/routes/reply.rs +++ b/crates/goose-server/src/routes/reply.rs @@ -123,7 +123,7 @@ async fn handler( let stream = ReceiverStream::new(rx); let messages = request.messages; - let session_working_dir = request.session_working_dir; + let session_working_dir = request.session_working_dir.clone(); let session_id = request .session_id @@ -181,7 +181,7 @@ async fn handler( &messages, Some(SessionConfig { id: session::Identifier::Name(session_id.clone()), - working_dir: PathBuf::from(session_working_dir), + working_dir: PathBuf::from(&session_working_dir), schedule_id: request.scheduled_job_id.clone(), execution_mode: None, max_turns: None, @@ -297,8 +297,13 @@ async fn handler( if all_messages.len() > saved_message_count { let provider = Arc::clone(provider.as_ref().unwrap()); tokio::spawn(async move { - if let Err(e) = - session::persist_messages(&session_path, &all_messages, Some(provider)).await + if let Err(e) = session::persist_messages( + &session_path, + &all_messages, + Some(provider), + Some(PathBuf::from(&session_working_dir)), + ) + .await { tracing::error!("Failed to store session history: {:?}", e); } @@ -337,7 +342,7 @@ async fn ask_handler( ) -> Result, StatusCode> { verify_secret_key(&headers, &state)?; - let session_working_dir = request.session_working_dir; + let session_working_dir = request.session_working_dir.clone(); let session_id = request .session_id @@ -358,7 +363,7 @@ async fn ask_handler( &messages, Some(SessionConfig { id: session::Identifier::Name(session_id.clone()), - working_dir: PathBuf::from(session_working_dir), + working_dir: PathBuf::from(&session_working_dir), schedule_id: request.scheduled_job_id.clone(), execution_mode: None, max_turns: None, @@ -420,9 +425,15 @@ async fn ask_handler( let session_path_clone = session_path.clone(); let messages = all_messages.clone(); let provider = Arc::clone(provider.as_ref().unwrap()); + let session_working_dir_clone = session_working_dir.clone(); tokio::spawn(async move { - if let Err(e) = - session::persist_messages(&session_path_clone, &messages, Some(provider)).await + if let Err(e) = session::persist_messages( + &session_path_clone, + &messages, + Some(provider), + Some(PathBuf::from(session_working_dir_clone)), + ) + .await { tracing::error!("Failed to store session history: {:?}", e); } diff --git a/crates/goose/src/session/storage.rs b/crates/goose/src/session/storage.rs index ad9d0096..81fd0ff1 100644 --- a/crates/goose/src/session/storage.rs +++ b/crates/goose/src/session/storage.rs @@ -1042,13 +1042,13 @@ pub fn read_metadata(session_file: &Path) -> Result { /// /// Security features: /// - Validates file paths to prevent directory traversal -/// - Uses secure file operations via persist_messages_with_schedule_id pub async fn persist_messages( session_file: &Path, messages: &[Message], provider: Option>, + working_dir: Option, ) -> Result<()> { - persist_messages_with_schedule_id(session_file, messages, provider, None).await + persist_messages_with_schedule_id(session_file, messages, provider, None, working_dir).await } /// Write messages to a session file with metadata, including an optional scheduled job ID @@ -1065,6 +1065,7 @@ pub async fn persist_messages_with_schedule_id( messages: &[Message], provider: Option>, schedule_id: Option, + working_dir: Option, ) -> Result<()> { // Validate the session file path for security let secure_path = get_path(Identifier::Path(session_file.to_path_buf()))?; @@ -1085,16 +1086,35 @@ 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, + working_dir, + ) + .await } _ => { - // Read existing metadata - let mut metadata = read_metadata(&secure_path)?; + // Read existing metadata or create new with proper working_dir + let mut metadata = if secure_path.exists() { + read_metadata(&secure_path)? + } else { + // Create new metadata with the provided working_dir or fall back to home + let work_dir = working_dir.clone().unwrap_or_else(get_home_dir); + SessionMetadata::new(work_dir) + }; + + // Update the working_dir if provided (even for existing files) + if let Some(work_dir) = working_dir { + metadata.working_dir = work_dir; + } + // Update the schedule_id if provided if schedule_id.is_some() { metadata.schedule_id = schedule_id; } + // Write the file with metadata and messages save_messages_with_metadata(&secure_path, &metadata, messages) } @@ -1232,11 +1252,12 @@ pub async fn generate_description( session_file: &Path, messages: &[Message], provider: Arc, + working_dir: Option, ) -> Result<()> { - generate_description_with_schedule_id(session_file, messages, provider, None).await + generate_description_with_schedule_id(session_file, messages, provider, None, working_dir).await } -/// Generate a description for the session using the provider, including an optional scheduled job ID +/// Generate a description for the session using the provider, including an optional scheduled job ID and working directory /// /// This function is called when appropriate to generate a short description /// of the session based on the conversation history. @@ -1250,6 +1271,7 @@ pub async fn generate_description_with_schedule_id( messages: &[Message], provider: Arc, schedule_id: Option, + working_dir: Option, ) -> Result<()> { // Validate the path for security let secure_path = get_path(Identifier::Path(session_file.to_path_buf()))?; @@ -1311,7 +1333,14 @@ pub async fn generate_description_with_schedule_id( description }; - let mut metadata = read_metadata(&secure_path)?; + // Create metadata with proper working_dir or read existing and update + let mut metadata = if secure_path.exists() { + read_metadata(&secure_path)? + } else { + // Create new metadata with the provided working_dir or fall back to home + let work_dir = working_dir.clone().unwrap_or_else(get_home_dir); + SessionMetadata::new(work_dir) + }; // Update description and schedule_id metadata.description = sanitized_description; @@ -1319,6 +1348,11 @@ pub async fn generate_description_with_schedule_id( metadata.schedule_id = schedule_id; } + // Update the working_dir if provided (even for existing files) + if let Some(work_dir) = working_dir { + metadata.working_dir = work_dir; + } + // Update the file with the new metadata and existing messages save_messages_with_metadata(&secure_path, &metadata, messages) } @@ -1430,7 +1464,7 @@ mod tests { ]; // Write messages - persist_messages(&file_path, &messages, None).await?; + persist_messages(&file_path, &messages, None, None).await?; // Read them back let read_messages = read_messages(&file_path)?; @@ -1538,7 +1572,7 @@ mod tests { } // Write messages with special characters - persist_messages(&file_path, &messages, None).await?; + persist_messages(&file_path, &messages, None, None).await?; // Read them back let read_messages = read_messages(&file_path)?; @@ -1603,7 +1637,7 @@ mod tests { ]; // Write messages - persist_messages(&file_path, &messages, None).await?; + persist_messages(&file_path, &messages, None, None).await?; // Read them back - should be truncated let read_messages = read_messages(&file_path)?; @@ -1694,6 +1728,162 @@ mod tests { Ok(()) } + #[tokio::test] + async fn test_working_dir_preservation() -> Result<()> { + let dir = tempdir()?; + let file_path = dir.path().join("test.jsonl"); + + // Create a temporary working directory + let working_dir = tempdir()?; + let working_dir_path = working_dir.path().to_path_buf(); + + // Create messages + let messages = vec![Message::user().with_text("test message")]; + + // Use persist_messages_with_schedule_id to set working dir + persist_messages_with_schedule_id( + &file_path, + &messages, + None, + None, + Some(working_dir_path.clone()), + ) + .await?; + + // Read back the metadata and verify working_dir is preserved + let metadata = read_metadata(&file_path)?; + assert_eq!(metadata.working_dir, working_dir_path); + + // Verify the messages are also preserved + let read_messages = read_messages(&file_path)?; + assert_eq!(read_messages.len(), 1); + assert_eq!(read_messages[0].role, messages[0].role); + + Ok(()) + } + + #[tokio::test] + async fn test_working_dir_issue_fixed() -> Result<()> { + // This test demonstrates that the working_dir issue in jsonl files is fixed + let dir = tempdir()?; + let file_path = dir.path().join("test.jsonl"); + + // Create a temporary working directory (this simulates the actual working directory) + let working_dir = tempdir()?; + let working_dir_path = working_dir.path().to_path_buf(); + + // Create messages + let messages = vec![Message::user().with_text("test message")]; + + // Get the home directory for comparison + let home_dir = get_home_dir(); + + // Test 1: Using the old persist_messages function (without working_dir) + // This will fall back to home directory since no working_dir is provided + persist_messages(&file_path, &messages, None, None).await?; + + // Read back the metadata - this should now have the home directory as working_dir + let metadata_old = read_metadata(&file_path)?; + assert_eq!( + metadata_old.working_dir, home_dir, + "persist_messages should use home directory when no working_dir is provided" + ); + + // Test 2: Using persist_messages_with_schedule_id function + // This should properly set the working_dir (this is the main fix) + persist_messages_with_schedule_id( + &file_path, + &messages, + None, + None, + Some(working_dir_path.clone()), + ) + .await?; + + // Read back the metadata - this should now have the correct working_dir + let metadata_new = read_metadata(&file_path)?; + assert_eq!( + metadata_new.working_dir, working_dir_path, + "persist_messages_with_schedule_id should use provided working_dir" + ); + assert_ne!( + metadata_new.working_dir, home_dir, + "working_dir should be different from home directory" + ); + + // Test 3: Create a new session file without working_dir (should fall back to home) + let file_path_2 = dir.path().join("test2.jsonl"); + persist_messages_with_schedule_id( + &file_path_2, + &messages, + None, + None, + None, // No working_dir provided + ) + .await?; + + let metadata_fallback = read_metadata(&file_path_2)?; + assert_eq!(metadata_fallback.working_dir, home_dir, "persist_messages_with_schedule_id should fall back to home directory when no working_dir is provided"); + + // Test 4: Test that the fix works for existing files + // Create a session file and then add to it with different working_dir + let file_path_3 = dir.path().join("test3.jsonl"); + + // First, create with home directory + persist_messages(&file_path_3, &messages, None, None).await?; + let metadata_initial = read_metadata(&file_path_3)?; + assert_eq!( + metadata_initial.working_dir, home_dir, + "Initial session should use home directory" + ); + + // Then update with a specific working_dir + persist_messages_with_schedule_id( + &file_path_3, + &messages, + None, + None, + Some(working_dir_path.clone()), + ) + .await?; + + let metadata_updated = read_metadata(&file_path_3)?; + assert_eq!( + metadata_updated.working_dir, working_dir_path, + "Updated session should use new working_dir" + ); + + // Test 5: Most important test - simulate the real-world scenario where + // CLI and web interfaces pass the current directory instead of None + let file_path_4 = dir.path().join("test4.jsonl"); + let current_dir = std::env::current_dir()?; + + // This is what web.rs and session/mod.rs do now after the fix + persist_messages_with_schedule_id( + &file_path_4, + &messages, + None, + None, + Some(current_dir.clone()), + ) + .await?; + + let metadata_current = read_metadata(&file_path_4)?; + assert_eq!( + metadata_current.working_dir, current_dir, + "Session should use current directory when explicitly provided" + ); + // This should NOT be the home directory anymore (unless current_dir == home_dir) + if current_dir != home_dir { + assert_ne!( + metadata_current.working_dir, home_dir, + "working_dir should be different from home directory when current_dir is different" + ); + } + + Ok(()) + } + #[test] fn test_windows_path_validation() -> Result<()> { // Test the Windows path validation logic @@ -1781,12 +1971,13 @@ mod tests { Message::assistant().with_text("Test response"), ]; - // Test persist_messages_with_schedule_id with save_session = true + // Test persist_messages_with_schedule_id with working_dir parameter persist_messages_with_schedule_id( &file_path, &messages, None, Some("test_schedule".to_string()), + None, ) .await?;