Defend against invalid sessions (#3229)

Co-authored-by: Douwe Osinga <douwe@squareup.com>

crates/goose-cli/src/commands/session.rs

@@ -1,7 +1,7 @@
 use crate::session::message_to_markdown;
 use anyhow::{Context, Result};
 use cliclack::{confirm, multiselect, select};
-use goose::session::info::{get_session_info, SessionInfo, SortOrder};
+use goose::session::info::{get_valid_sorted_sessions, SessionInfo, SortOrder};
 use goose::session::{self, Identifier};
 use regex::Regex;
 use std::fs;
@@ -75,7 +75,7 @@ fn prompt_interactive_session_removal(sessions: &[SessionInfo]) -> Result<Vec<Se
 }
 
 pub fn handle_session_remove(id: Option<String>, regex_string: Option<String>) -> Result<()> {
-    let all_sessions = match get_session_info(SortOrder::Descending) {
+    let all_sessions = match get_valid_sorted_sessions(SortOrder::Descending) {
         Ok(sessions) => sessions,
         Err(e) => {
             tracing::error!("Failed to retrieve sessions: {:?}", e);
@@ -125,7 +125,7 @@ pub fn handle_session_list(verbose: bool, format: String, ascending: bool) -> Re
         SortOrder::Descending
     };
 
-    let sessions = match get_session_info(sort_order) {
+    let sessions = match get_valid_sorted_sessions(sort_order) {
         Ok(sessions) => sessions,
         Err(e) => {
             tracing::error!("Failed to list sessions: {:?}", e);
@@ -294,7 +294,7 @@ fn export_session_to_markdown(
 /// Shows a list of available sessions and lets the user select one
 pub fn prompt_interactive_session_selection() -> Result<session::Identifier> {
     // Get sessions sorted by modification date (newest first)
-    let sessions = match get_session_info(SortOrder::Descending) {
+    let sessions = match get_valid_sorted_sessions(SortOrder::Descending) {
         Ok(sessions) => sessions,
         Err(e) => {
             tracing::error!("Failed to list sessions: {:?}", e);

crates/goose-cli/src/commands/web.rs

@@ -225,14 +225,15 @@ async fn list_sessions() -> Json<serde_json::Value> {
         Ok(sessions) => {
             let session_info: Vec<serde_json::Value> = sessions
                 .into_iter()
-                .map(|(name, path)| {
+                .filter_map(|(name, path)| {
-                    let metadata = session::read_metadata(&path).unwrap_or_default();
+                    session::read_metadata(&path).ok().map(|metadata| {
-                    serde_json::json!({
+                        serde_json::json!({
-                        "name": name,
+                            "name": name,
-                        "path": path,
+                            "path": path,
-                        "description": metadata.description,
+                            "description": metadata.description,
-                        "message_count": metadata.message_count,
+                            "message_count": metadata.message_count,
-                        "working_dir": metadata.working_dir
+                            "working_dir": metadata.working_dir
+                        })
                     })
                 })
                 .collect();
@@ -246,7 +247,6 @@ async fn list_sessions() -> Json<serde_json::Value> {
         })),
     }
 }
-
 async fn get_session(
     axum::extract::Path(session_id): axum::extract::Path<String>,
 ) -> Json<serde_json::Value> {
@@ -259,17 +259,21 @@ async fn get_session(
         }
     };
 
+    let error_response = |e: Box<dyn std::error::Error>| {
+        Json(serde_json::json!({
+            "error": e.to_string()
+        }))
+    };
+
     match session::read_messages(&session_file) {
-        Ok(messages) => {
+        Ok(messages) => match session::read_metadata(&session_file) {
-            let metadata = session::read_metadata(&session_file).unwrap_or_default();
+            Ok(metadata) => Json(serde_json::json!({
-            Json(serde_json::json!({
                 "metadata": metadata,
                 "messages": messages
-            }))
+            })),
-        }
+            Err(e) => error_response(e.into()),
-        Err(e) => Json(serde_json::json!({
+        },
-            "error": e.to_string()
+        Err(e) => error_response(e.into()),
-        })),
     }
 }
 

crates/goose-cli/src/session/mod.rs

@@ -1259,7 +1259,6 @@ impl Session {
         );
     }
 
-    /// Get the session metadata
     pub fn get_metadata(&self) -> Result<session::SessionMetadata> {
         if !self.session_file.as_ref().is_some_and(|f| f.exists()) {
             return Err(anyhow::anyhow!("Session file does not exist"));

crates/goose-server/src/routes/session.rs

@@ -10,7 +10,7 @@ use axum::{
 };
 use goose::message::Message;
 use goose::session;
-use goose::session::info::{get_session_info, SessionInfo, SortOrder};
+use goose::session::info::{get_valid_sorted_sessions, SessionInfo, SortOrder};
 use goose::session::SessionMetadata;
 use serde::Serialize;
 use utoipa::ToSchema;
@@ -53,8 +53,8 @@ async fn list_sessions(
 ) -> Result<Json<SessionListResponse>, StatusCode> {
     verify_secret_key(&headers, &state)?;
 
-    let sessions =
+    let sessions = get_valid_sorted_sessions(SortOrder::Descending)
-        get_session_info(SortOrder::Descending).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
+        .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
 
     Ok(Json(SessionListResponse { sessions }))
 }
@@ -89,7 +89,6 @@ async fn get_session_history(
         Err(_) => return Err(StatusCode::BAD_REQUEST),
     };
 
-    // Read metadata
     let metadata = session::read_metadata(&session_path).map_err(|_| StatusCode::NOT_FOUND)?;
 
     let messages = match session::read_messages(&session_path) {

crates/goose/src/session/info.rs

@@ -18,7 +18,7 @@ pub enum SortOrder {
     Descending,
 }
 
-pub fn get_session_info(sort_order: SortOrder) -> Result<Vec<SessionInfo>> {
+pub fn get_valid_sorted_sessions(sort_order: SortOrder) -> Result<Vec<SessionInfo>> {
     let sessions = match session::list_sessions() {
         Ok(sessions) => sessions,
         Err(e) => {
@@ -26,10 +26,9 @@ pub fn get_session_info(sort_order: SortOrder) -> Result<Vec<SessionInfo>> {
             return Err(anyhow::anyhow!("Failed to list sessions"));
         }
     };
-    let mut session_infos = sessions
+    let mut session_infos: Vec<SessionInfo> = sessions
         .into_iter()
-        .map(|(id, path)| {
+        .filter_map(|(id, path)| {
-            // Get last modified time as string
             let modified = path
                 .metadata()
                 .and_then(|m| m.modified())
@@ -38,19 +37,18 @@ pub fn get_session_info(sort_order: SortOrder) -> Result<Vec<SessionInfo>> {
                         .format("%Y-%m-%d %H:%M:%S UTC")
                         .to_string()
                 })
-                .unwrap_or_else(|_| "Unknown".to_string());
+                .ok()?;
 
-            // Get session description
+            let metadata = session::read_metadata(&path).ok()?;
-            let metadata = session::read_metadata(&path).expect("Failed to read session metadata");
 
-            SessionInfo {
+            Some(SessionInfo {
                 id,
                 path: path.to_string_lossy().to_string(),
                 modified,
                 metadata,
-            }
+            })
         })
-        .collect::<Vec<SessionInfo>>();
+        .collect();
 
     // Sort sessions by modified date
     // Since all dates are in ISO format (YYYY-MM-DD HH:MM:SS UTC), we can just use string comparison

crates/goose/src/session/mod.rs

@@ -9,4 +9,4 @@ pub use storage::{
     SessionMetadata,
 };
 
-pub use info::{get_session_info, SessionInfo};
+pub use info::{get_valid_sorted_sessions, SessionInfo};

crates/goose/src/session/storage.rs

@@ -1309,7 +1309,6 @@ pub async fn generate_description_with_schedule_id(
         description
     };
 
-    // Read current metadata
     let mut metadata = read_metadata(&secure_path)?;
 
     // Update description and schedule_id