diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 2b066197af..3afed8693c 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -1,6 +1,7 @@ use clap::Args; use clap::CommandFactory; use clap::Parser; +use clap::ValueHint; use clap_complete::Shell; use clap_complete::generate; use codex_arg0::arg0_dispatch_or_else; @@ -37,6 +38,8 @@ use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::features::is_known_feature_key; +const RESUME_LAST_PROMPT_SENTINEL: &str = "__codex_resume_last_flag__"; + /// Codex CLI /// /// If no subcommand is specified, options will be forwarded to the interactive CLI. @@ -135,9 +138,17 @@ struct ResumeCommand { #[arg(value_name = "SESSION_ID")] session_id: Option, - /// Continue the most recent session without showing the picker. - #[arg(long = "last", default_value_t = false, conflicts_with = "session_id")] - last: bool, + /// Continue the most recent session without showing the picker. Optionally provide a prompt + /// right after the flag (e.g. `--last `). + #[arg( + long = "last", + value_name = "PROMPT", + value_hint = ValueHint::Other, + num_args = 0..=1, + default_missing_value = RESUME_LAST_PROMPT_SENTINEL, + conflicts_with = "session_id" + )] + last: Option, /// Show all sessions (disables cwd filtering and shows CWD column). #[arg(long = "all", default_value_t = false)] @@ -641,21 +652,32 @@ fn finalize_resume_interactive( mut interactive: TuiCli, root_config_overrides: CliConfigOverrides, session_id: Option, - last: bool, + last: Option, show_all: bool, resume_cli: TuiCli, ) -> TuiCli { // Start with the parsed interactive CLI so resume shares the same // configuration surface area as `codex` without additional flags. let resume_session_id = session_id; - interactive.resume_picker = resume_session_id.is_none() && !last; - interactive.resume_last = last; + let resume_last = last.is_some(); + let last_prompt = last + .as_deref() + .filter(|value| *value != RESUME_LAST_PROMPT_SENTINEL) + .map(str::to_owned); + interactive.resume_picker = resume_session_id.is_none() && !resume_last; + interactive.resume_last = resume_last; interactive.resume_session_id = resume_session_id; interactive.resume_show_all = show_all; // Merge resume-scoped flags and overrides with highest precedence. merge_resume_cli_flags(&mut interactive, resume_cli); + if interactive.prompt.is_none() + && let Some(prompt) = last_prompt + { + interactive.prompt = Some(prompt); + } + // Propagate any root-level config overrides (e.g. `-c key=value`). prepend_config_flags(&mut interactive.config_overrides, root_config_overrides); @@ -828,6 +850,59 @@ mod tests { assert!(!interactive.resume_show_all); } + #[test] + fn resume_last_accepts_prompt_after_flag() { + let interactive = finalize_from_args(["codex", "resume", "--last", "next steps"].as_ref()); + assert!(interactive.resume_last); + assert_eq!(interactive.prompt.as_deref(), Some("next steps")); + } + + #[test] + fn resume_last_rejects_uuid_with_last_flag() { + // Interactive mode uses conflicts_with, so clap should reject this combination + let result = MultitoolCli::try_parse_from([ + "codex", + "resume", + "123e4567-e89b-12d3-a456-426614174000", + "--last", + ]); + assert!(result.is_err(), "Should reject UUID with --last flag"); + let err_msg = result.unwrap_err().to_string(); + assert!( + err_msg.contains("cannot be used") || err_msg.contains("conflicts"), + "Error message should mention conflict: {err_msg}" + ); + } + + #[test] + fn resume_last_rejects_uuid_with_last_flag_and_prompt() { + // Test UUID with --last and prompt value + let result = MultitoolCli::try_parse_from([ + "codex", + "resume", + "123e4567-e89b-12d3-a456-426614174000", + "--last", + "prompt", + ]); + assert!( + result.is_err(), + "Should reject UUID with --last flag even with prompt" + ); + } + + #[test] + fn resume_prompt_before_last_flag_parsed_as_session_id() { + // When prompt comes before --last, clap parses it as session_id + // But since it's not a UUID and conflicts_with is set, it should still error + // This tests the edge case mentioned in the issue comment + let result = MultitoolCli::try_parse_from(["codex", "resume", "2+2", "--last"]); + // With conflicts_with, clap will reject this because session_id is set + assert!( + result.is_err(), + "Should reject when positional before --last conflicts" + ); + } + #[test] fn resume_picker_logic_with_session_id() { let interactive = finalize_from_args(["codex", "resume", "1234"].as_ref()); diff --git a/codex-rs/exec/src/cli.rs b/codex-rs/exec/src/cli.rs index 6866bc0ff7..5850411b92 100644 --- a/codex-rs/exec/src/cli.rs +++ b/codex-rs/exec/src/cli.rs @@ -3,6 +3,8 @@ use clap::ValueEnum; use codex_common::CliConfigOverrides; use std::path::PathBuf; +const RESUME_LAST_PROMPT_SENTINEL: &str = "__codex_resume_last_flag__"; + #[derive(Parser, Debug)] #[command(version)] pub struct Cli { @@ -101,8 +103,16 @@ pub struct ResumeArgs { pub session_id: Option, /// Resume the most recent recorded session (newest) without specifying an id. - #[arg(long = "last", default_value_t = false)] - pub last: bool, + /// Optionally accepts a prompt directly after the flag (e.g. `--last `). + #[arg( + long = "last", + value_name = "PROMPT", + value_hint = clap::ValueHint::Other, + num_args = 0..=1, + default_missing_value = "__codex_resume_last_flag__", + conflicts_with = "session_id" + )] + pub last: Option, /// Prompt to send after resuming the session. If `-` is used, read from stdin. #[arg(value_name = "PROMPT", value_hint = clap::ValueHint::Other)] @@ -117,3 +127,15 @@ pub enum Color { #[default] Auto, } + +impl ResumeArgs { + pub fn resume_last(&self) -> bool { + self.last.is_some() + } + + pub fn last_prompt(&self) -> Option<&str> { + self.last + .as_deref() + .filter(|value| *value != RESUME_LAST_PROMPT_SENTINEL) + } +} diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index caf76baeb5..cd74469593 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -87,15 +87,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any let resume_prompt = args .prompt .clone() - // When using `resume --last `, clap still parses the first positional - // as `session_id`. Reinterpret it as the prompt so the flag works with JSON mode. - .or_else(|| { - if args.last { - args.session_id.clone() - } else { - None - } - }); + .or_else(|| args.last_prompt().map(str::to_owned)); resume_prompt.or(prompt) } None => prompt, @@ -336,7 +328,12 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any conversation_manager .resume_conversation_from_rollout(config.clone(), path, auth_manager.clone()) .await? + } else if let Some(id_str) = args.session_id.as_deref() { + // If a session_id was explicitly provided but not found, error out. + eprintln!("No saved session found with ID {id_str}."); + std::process::exit(1); } else { + // No session_id provided and --last didn't find anything, start fresh. conversation_manager .new_conversation(config.clone()) .await? @@ -452,7 +449,9 @@ async fn resolve_resume_path( config: &Config, args: &crate::cli::ResumeArgs, ) -> anyhow::Result> { - if args.last { + if args.resume_last() { + // If --last is present, use it to find the most recent session. + // With conflicts_with, session_id cannot be present when --last is set. let default_provider_filter = vec![config.model_provider_id.clone()]; match codex_core::RolloutRecorder::list_conversations( &config.codex_home, diff --git a/codex-rs/exec/tests/suite/mod.rs b/codex-rs/exec/tests/suite/mod.rs index 77012ee3b7..330249a285 100644 --- a/codex-rs/exec/tests/suite/mod.rs +++ b/codex-rs/exec/tests/suite/mod.rs @@ -5,5 +5,6 @@ mod auth_env; mod originator; mod output_schema; mod resume; +mod resume_verification; mod sandbox; mod server_error_exit; diff --git a/codex-rs/exec/tests/suite/resume.rs b/codex-rs/exec/tests/suite/resume.rs index e37b38606a..70662b11f7 100644 --- a/codex-rs/exec/tests/suite/resume.rs +++ b/codex-rs/exec/tests/suite/resume.rs @@ -1,6 +1,8 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] use anyhow::Context; use core_test_support::test_codex_exec::test_codex_exec; +use predicates::prelude::*; +use predicates::str::contains; use serde_json::Value; use std::path::Path; use std::string::ToString; @@ -177,6 +179,26 @@ fn exec_resume_last_accepts_prompt_after_flag_in_json_mode() -> anyhow::Result<( Ok(()) } +#[test] +fn exec_resume_last_conflicts_with_session_id() -> anyhow::Result<()> { + let test = test_codex_exec(); + let session_id = Uuid::new_v4().to_string(); + + // With conflicts_with, clap rejects this combination immediately + test.cmd() + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("resume") + .arg(&session_id) + .arg("--last") + .assert() + .failure() + .stderr(contains("cannot be used with").or(contains("conflicts"))); + + Ok(()) +} + #[test] fn exec_resume_by_id_appends_to_existing_file() -> anyhow::Result<()> { let test = test_codex_exec(); @@ -309,3 +331,139 @@ fn exec_resume_preserves_cli_configuration_overrides() -> anyhow::Result<()> { assert!(content.contains(&marker2)); Ok(()) } + +#[test] +fn exec_resume_last_with_prompt_as_positional_after_flag() -> anyhow::Result<()> { + let test = test_codex_exec(); + let fixture = + Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/cli_responses_fixture.sse"); + + // 1) First run: create a session + let marker = format!("resume-last-positional-{}", Uuid::new_v4()); + let prompt = format!("echo {marker}"); + + test.cmd() + .env("CODEX_RS_SSE_FIXTURE", &fixture) + .env("OPENAI_BASE_URL", "http://unused.local") + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg(&prompt) + .assert() + .success(); + + let sessions_dir = test.home_path().join("sessions"); + let path = find_session_file_containing_marker(&sessions_dir, &marker) + .expect("no session file found after first run"); + + // 2) Resume with --last and prompt as separate positional (simulating JSON mode issue) + let marker2 = format!("resume-last-positional-2-{}", Uuid::new_v4()); + let prompt2 = format!("echo {marker2}"); + + // This simulates: codex exec resume --last "prompt" + // where clap might parse "prompt" as session_id instead of as --last value + test.cmd() + .env("CODEX_RS_SSE_FIXTURE", &fixture) + .env("OPENAI_BASE_URL", "http://unused.local") + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("resume") + .arg("--last") + .arg(&prompt2) + .assert() + .success(); + + let resumed_path = find_session_file_containing_marker(&sessions_dir, &marker2) + .expect("no resumed session file containing marker2"); + assert_eq!( + resumed_path, path, + "resume --last should append to existing file" + ); + let content = std::fs::read_to_string(&resumed_path)?; + assert!(content.contains(&marker)); + assert!(content.contains(&marker2)); + Ok(()) +} + +#[test] +fn exec_resume_last_rejects_real_uuid_as_session_id() -> anyhow::Result<()> { + let test = test_codex_exec(); + let session_id = Uuid::new_v4().to_string(); + + // This should fail because we're providing both --last and a real UUID as session_id + test.cmd() + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("resume") + .arg(&session_id) + .arg("--last") + .assert() + .failure() + .stderr(contains("cannot be used with")); + + Ok(()) +} + +#[test] +fn exec_resume_last_rejects_positional_before_flag() -> anyhow::Result<()> { + // With conflicts_with, clap rejects any positional argument before --last + // This ensures users use the documented form: codex exec resume --last "prompt" + let test = test_codex_exec(); + let prompt = "echo test"; + + test.cmd() + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("resume") + .arg(prompt) // This will be parsed as session_id by clap + .arg("--last") + .assert() + .failure() + .stderr(contains("cannot be used with").or(contains("conflicts"))); + + Ok(()) +} + +#[test] +fn exec_resume_rejects_invalid_session_id() -> anyhow::Result<()> { + let test = test_codex_exec(); + let invalid_id = "sdfsdfgfds2546456"; + + // Invalid session ID should fail with an error message + test.cmd() + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("resume") + .arg(invalid_id) + .arg("2+2") + .assert() + .failure() + .stderr(contains("No saved session found with ID")); + + Ok(()) +} + +#[test] +fn exec_resume_rejects_nonexistent_valid_uuid() -> anyhow::Result<()> { + let test = test_codex_exec(); + // Generate a valid UUID that doesn't exist + let nonexistent_id = Uuid::new_v4().to_string(); + + // Valid UUID format but not found should fail with an error message + test.cmd() + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("resume") + .arg(&nonexistent_id) + .arg("2+2") + .assert() + .failure() + .stderr(contains("No saved session found with ID")); + + Ok(()) +} diff --git a/codex-rs/exec/tests/suite/resume_verification.rs b/codex-rs/exec/tests/suite/resume_verification.rs new file mode 100644 index 0000000000..01c28cdf69 --- /dev/null +++ b/codex-rs/exec/tests/suite/resume_verification.rs @@ -0,0 +1,233 @@ +#![allow(clippy::unwrap_used, clippy::expect_used)] +use core_test_support::test_codex_exec::test_codex_exec; +use predicates::prelude::*; +use predicates::str::contains; +use serde_json::Value; +use std::path::Path; +use std::string::ToString; +use uuid::Uuid; +use walkdir::WalkDir; + +/// Utility: scan the sessions dir for a rollout file that contains `marker` +/// in any response_item.message.content entry. Returns the absolute path. +fn find_session_file_containing_marker( + sessions_dir: &std::path::Path, + marker: &str, +) -> Option { + for entry in WalkDir::new(sessions_dir) { + let entry = match entry { + Ok(e) => e, + Err(_) => continue, + }; + if !entry.file_type().is_file() { + continue; + } + if !entry.file_name().to_string_lossy().ends_with(".jsonl") { + continue; + } + let path = entry.path(); + let Ok(content) = std::fs::read_to_string(path) else { + continue; + }; + // Skip the first meta line and scan remaining JSONL entries. + let mut lines = content.lines(); + if lines.next().is_none() { + continue; + } + for line in lines { + if line.trim().is_empty() { + continue; + } + let Ok(item): Result = serde_json::from_str(line) else { + continue; + }; + if item.get("type").and_then(|t| t.as_str()) == Some("response_item") + && let Some(payload) = item.get("payload") + && payload.get("type").and_then(|t| t.as_str()) == Some("message") + && payload + .get("content") + .map(ToString::to_string) + .unwrap_or_default() + .contains(marker) + { + return Some(path.to_path_buf()); + } + } + } + None +} + +/// Test the exact scenario from issue #6717: +/// `codex exec --json resume --last "2+2"` should work +#[test] +fn exec_resume_last_json_mode_exact_issue_scenario() -> anyhow::Result<()> { + let test = test_codex_exec(); + let fixture = + Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/cli_responses_fixture.sse"); + + // 1) First run: create a session + let marker = format!("resume-json-issue-{}", Uuid::new_v4()); + let prompt = format!("echo {marker}"); + + test.cmd() + .env("CODEX_RS_SSE_FIXTURE", &fixture) + .env("OPENAI_BASE_URL", "http://unused.local") + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg(&prompt) + .assert() + .success(); + + let sessions_dir = test.home_path().join("sessions"); + let path = find_session_file_containing_marker(&sessions_dir, &marker) + .expect("no session file found after first run"); + + // 2) Test the exact command from issue #6717 + let marker2 = format!("resume-json-issue-2-{}", Uuid::new_v4()); + let prompt2 = format!("echo {marker2}"); + + // This is the exact command from the issue: codex exec --json resume --last "2+2" + // But we'll use a more testable prompt + test.cmd() + .env("CODEX_RS_SSE_FIXTURE", &fixture) + .env("OPENAI_BASE_URL", "http://unused.local") + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("--json") + .arg("resume") + .arg("--last") + .arg(&prompt2) + .assert() + .success(); + + let resumed_path = find_session_file_containing_marker(&sessions_dir, &marker2) + .expect("no resumed session file containing marker2"); + assert_eq!( + resumed_path, path, + "resume --last should append to existing file" + ); + let content = std::fs::read_to_string(&resumed_path)?; + assert!(content.contains(&marker)); + assert!(content.contains(&marker2)); + Ok(()) +} + +/// Test security issue: UUID with --last should error +#[test] +fn exec_resume_uuid_with_last_should_error() -> anyhow::Result<()> { + let test = test_codex_exec(); + let session_id = Uuid::new_v4().to_string(); + + // Test UUID with dashes - clap rejects this with conflicts_with + test.cmd() + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("resume") + .arg(&session_id) + .arg("--last") + .assert() + .failure() + .stderr(contains("cannot be used with").or(contains("conflicts"))); + + // Test UUID without dashes (32 hex chars) - clap rejects this with conflicts_with + let session_id_no_dashes = session_id.replace('-', ""); + test.cmd() + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("resume") + .arg(&session_id_no_dashes) + .arg("--last") + .assert() + .failure() + .stderr(contains("cannot be used with").or(contains("conflicts"))); + + Ok(()) +} + +/// Test that positional arguments before --last are rejected (simplified approach) +#[test] +fn exec_resume_rejects_positional_before_last() -> anyhow::Result<()> { + let test = test_codex_exec(); + + // With conflicts_with, clap rejects any positional argument before --last + // This ensures users use the documented form: codex exec resume --last "prompt" + let test_cases = vec![ + "2+2", // Original issue example - simple math + "hello world", // Simple text + "12345678-1234-1234-1234-123456789012", // Wrong format (too many chars) + "12345678-1234-1234-1234-1234567890", // Wrong format (too few chars) + ]; + + for non_uuid_string in test_cases { + // All of these should be rejected because clap treats them as session_id + // and conflicts_with prevents session_id and --last from being used together + test.cmd() + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg("resume") + .arg(non_uuid_string) + .arg("--last") + .assert() + .failure() + .stderr(contains("cannot be used with").or(contains("conflicts"))); + } + + Ok(()) +} + +/// Test edge case: --last with value vs without value +#[test] +fn exec_resume_last_with_and_without_value() -> anyhow::Result<()> { + let test = test_codex_exec(); + let fixture = + Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/cli_responses_fixture.sse"); + + // 1) First run: create a session + let marker = format!("resume-last-value-{}", Uuid::new_v4()); + let prompt = format!("echo {marker}"); + + test.cmd() + .env("CODEX_RS_SSE_FIXTURE", &fixture) + .env("OPENAI_BASE_URL", "http://unused.local") + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg(&prompt) + .assert() + .success(); + + let sessions_dir = test.home_path().join("sessions"); + let path = find_session_file_containing_marker(&sessions_dir, &marker) + .expect("no session file found after first run"); + + // 2) Test --last without value (should work, reads from parent prompt or stdin) + let marker2 = format!("resume-last-value-2-{}", Uuid::new_v4()); + let prompt2 = format!("echo {marker2}"); + + // --last without value, prompt provided at parent level + test.cmd() + .env("CODEX_RS_SSE_FIXTURE", &fixture) + .env("OPENAI_BASE_URL", "http://unused.local") + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg(&prompt2) + .arg("resume") + .arg("--last") + .assert() + .success(); + + let resumed_path = find_session_file_containing_marker(&sessions_dir, &marker2) + .expect("no resumed session file containing marker2"); + assert_eq!( + resumed_path, path, + "resume --last should append to existing file" + ); + + Ok(()) +}