fix: --session-id shouldn't work without --resume, but --name should (#5360)

crates/goose-cli/src/cli.rs

@@ -35,9 +35,9 @@ struct Cli {
     command: Option<Command>,
 }
 
-#[derive(Args, Debug)]
+#[derive(Args, Debug, Clone)]
 #[group(required = false, multiple = false)]
-struct Identifier {
+pub struct Identifier {
     #[arg(
         short,
         long,
@@ -46,7 +46,7 @@ struct Identifier {
         long_help = "Specify a name for your chat session. When used with --resume, will resume this specific session if it exists.",
         alias = "id"
     )]
-    name: Option<String>,
+    pub name: Option<String>,
 
     #[arg(
         long = "session-id",
@@ -54,7 +54,7 @@ struct Identifier {
         help = "Session ID (e.g., '20250921_143022')",
         long_help = "Specify a session ID directly. When used with --resume, will resume this specific session if it exists."
     )]
-    session_id: Option<String>,
+    pub session_id: Option<String>,
 
     #[arg(
         short,
@@ -64,15 +64,67 @@ struct Identifier {
         long_help = "Legacy parameter for backward compatibility. Extracts session ID from the file path (e.g., '/path/to/20250325_200615.
 jsonl' -> '20250325_200615')."
     )]
-    path: Option<PathBuf>,
+    pub path: Option<PathBuf>,
 }
 
-async fn get_session_id(identifier: Identifier) -> Result<String> {
+async fn get_or_create_session_id(
+    identifier: Option<Identifier>,
+    resume: bool,
+    no_session: bool,
+) -> Result<Option<String>> {
+    if no_session {
+        return Ok(None);
+    }
+
+    let Some(id) = identifier else {
+        let session =
+            SessionManager::create_session(std::env::current_dir()?, "CLI Session".to_string())
+                .await?;
+        return Ok(Some(session.id));
+    };
+
+    if let Some(session_id) = id.session_id {
+        Ok(Some(session_id))
+    } else if let Some(name) = id.name {
+        if resume {
+            let sessions = SessionManager::list_sessions().await?;
+            let session_id = sessions
+                .into_iter()
+                .find(|s| s.name == name || s.id == name)
+                .map(|s| s.id)
+                .ok_or_else(|| anyhow::anyhow!("No session found with name '{}'", name))?;
+            Ok(Some(session_id))
+        } else {
+            let session =
+                SessionManager::create_session(std::env::current_dir()?, name.clone()).await?;
+
+            SessionManager::update_session(&session.id)
+                .user_provided_name(name)
+                .apply()
+                .await?;
+
+            Ok(Some(session.id))
+        }
+    } else if let Some(path) = id.path {
+        let session_id = path
+            .file_stem()
+            .and_then(|s| s.to_str())
+            .map(|s| s.to_string())
+            .ok_or_else(|| anyhow::anyhow!("Could not extract session ID from path: {:?}", path))?;
+        Ok(Some(session_id))
+    } else {
+        let session =
+            SessionManager::create_session(std::env::current_dir()?, "CLI Session".to_string())
+                .await?;
+        Ok(Some(session.id))
+    }
+}
+
+async fn lookup_session_id(identifier: Identifier) -> Result<String> {
     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
             .into_iter()
             .find(|s| s.name == name || s.id == name)
@@ -84,9 +136,10 @@ async fn get_session_id(identifier: Identifier) -> Result<String> {
             .map(|s| s.to_string())
             .ok_or_else(|| anyhow::anyhow!("Could not extract session ID from path: {:?}", path))
     } else {
-        unreachable!()
+        Err(anyhow::anyhow!("No identifier provided"))
     }
 }
+
 fn parse_key_val(s: &str) -> Result<(String, String), String> {
     match s.split_once('=') {
         Some((key, value)) => Ok((key.to_string(), value.to_string())),
@@ -836,7 +889,7 @@ pub async fn cli() -> Result<()> {
                     format,
                 }) => {
                     let session_identifier = if let Some(id) = identifier {
-                        get_session_id(id).await?
+                        lookup_session_id(id).await?
                     } else {
                         // If no identifier is provided, prompt for interactive selection
                         match crate::commands::session::prompt_interactive_session_selection().await
@@ -872,11 +925,18 @@ pub async fn cli() -> Result<()> {
                         "Session started"
                     );
 
-                    let session_id = if let Some(id) = identifier {
+                    if let Some(Identifier {
-                        Some(get_session_id(id).await?)
+                        session_id: Some(_),
-                    } else {
+                        ..
-                        None
+                    }) = &identifier
-                    };
+                    {
+                        if !resume {
+                            eprintln!("Error: --session-id can only be used with --resume flag");
+                            std::process::exit(1);
+                        }
+                    }
+
+                    let session_id = get_or_create_session_id(identifier, resume, false).await?;
 
                     // Run session command by default
                     let mut session: crate::CliSession = build_session(SessionBuilderConfig {
@@ -1070,11 +1130,19 @@ pub async fn cli() -> Result<()> {
                     std::process::exit(1);
                 }
             };
-            let session_id = if let Some(id) = identifier {
+
-                Some(get_session_id(id).await?)
+            if let Some(Identifier {
-            } else {
+                session_id: Some(_),
-                None
+                ..
-            };
+            }) = &identifier
+            {
+                if !resume {
+                    eprintln!("Error: --session-id can only be used with --resume flag");
+                    std::process::exit(1);
+                }
+            }
+
+            let session_id = get_or_create_session_id(identifier, resume, no_session).await?;
 
             let mut session = build_session(SessionBuilderConfig {
                 session_id,
@@ -1261,8 +1329,10 @@ pub async fn cli() -> Result<()> {
                 Ok(())
             } else {
                 // Run session command by default
+                let session_id = get_or_create_session_id(None, false, false).await?;
+
                 let mut session = build_session(SessionBuilderConfig {
-                    session_id: None,
+                    session_id,
                     resume: false,
                     no_session: false,
                     extensions: Vec::new(),

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

@@ -25,7 +25,7 @@ use tokio::task::JoinSet;
 /// including session identification, extension configuration, and debug settings.
 #[derive(Default, Clone, Debug)]
 pub struct SessionBuilderConfig {
-    /// Optional identifier for the session
+    /// Optional session ID for resuming or identifying an existing session
     pub session_id: Option<String>,
     /// Whether to resume an existing session
     pub resume: bool,
@@ -278,7 +278,6 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> CliSession {
             process::exit(1);
         });
 
-    // Handle session resolution and resuming
     let session_id: Option<String> = if session_config.no_session {
         None
     } else if session_config.resume {
@@ -295,29 +294,15 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> CliSession {
             }
         } else {
             match SessionManager::list_sessions().await {
-                Ok(sessions) => {
+                Ok(sessions) if !sessions.is_empty() => Some(sessions[0].id.clone()),
-                    if sessions.is_empty() {
+                _ => {
-                        output::render_error("Cannot resume - no previous sessions found");
-                        process::exit(1);
-                    }
-                    Some(sessions[0].id.clone())
-                }
-                Err(_) => {
                     output::render_error("Cannot resume - no previous sessions found");
                     process::exit(1);
                 }
             }
         }
-    } else if let Some(session_id) = session_config.session_id {
-        Some(session_id)
     } else {
-        let session = SessionManager::create_session(
+        session_config.session_id
-            std::env::current_dir().unwrap(),
-            "CLI Session".to_string(),
-        )
-        .await
-        .unwrap();
-        Some(session.id)
     };
 
     agent
@@ -331,7 +316,6 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> CliSession {
 
     if session_config.resume {
         if let Some(session_id) = session_id.as_ref() {
-            // Read the session metadata from database
             let metadata = SessionManager::get_session(session_id, false)
                 .await
                 .unwrap_or_else(|e| {
@@ -342,7 +326,6 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> CliSession {
             let current_workdir =
                 std::env::current_dir().expect("Failed to get current working directory");
             if current_workdir != metadata.working_dir {
-                // Ask user if they want to change the working directory
                 let change_workdir = cliclack::confirm(format!("{} The original working directory of this session was set to {}. Your current directory is {}. Do you want to switch back to the original working directory?", style("WARNING:").yellow(), style(metadata.working_dir.display()).cyan(), style(current_workdir.display()).cyan()))
                     .initial_value(true)
                     .interact().expect("Failed to get user input");
@@ -634,7 +617,7 @@ mod tests {
     #[test]
     fn test_session_builder_config_creation() {
         let config = SessionBuilderConfig {
-            session_id: Some("test".to_string()),
+            session_id: None,
             resume: false,
             no_session: false,
             extensions: vec!["echo test".to_string()],