Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion codex-rs/exec/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ pub struct ResumeArgs {
pub session_id: Option<String>,

/// Resume the most recent recorded session (newest) without specifying an id.
#[arg(long = "last", default_value_t = false, conflicts_with = "session_id")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping conflicts_with = "session_id" was safe here:

  • The only time clap used that constraint was to reject resume --last when was parsed as SESSION_ID. We now re-interpret that positional as the prompt whenever --last is set.

  • Users can still run resume <SESSION_ID> without --last; the parser still sets session_id and leaves last false, so nothing changes for that flow.

  • If someone deliberately provides both an ID and --last, we prefer the session_id path because we only treat the positional as a prompt when last is true and no explicit prompt was supplied. So accidental conflicts disappear, and intentional resume --last -- session-id (if someone tried) still resolves consistently.

Clap doesn’t need the explicit conflicts_with once the parser logic distinguishes the cases.

#[arg(long = "last", default_value_t = false)]
pub last: bool,

/// Prompt to send after resuming the session. If `-` is used, read from stdin.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore --last/session_id conflict

Removing conflicts_with = "session_id" means clap will now happily parse codex exec resume <SESSION_ID> --last. With the new prompt fallback we even reinterpret that <SESSION_ID> as the prompt, but resolve_resume_path still treats args.last as authoritative and ignores the provided id. The net result is that this command (which used to error) silently resumes the most recent conversation instead of the explicit session the user asked for, and sends the session id string as the first prompt. That is a regression in CLI safety—users/scripts that accidentally pass both options now touch the wrong session. We need to keep the conflict (or otherwise reject the combination) while fixing the JSON-mode prompt parsing.

Useful? React with 👍 / 👎.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, but I'm not sure how to express this more cleanly with clap.

Expand Down
16 changes: 15 additions & 1 deletion codex-rs/exec/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,21 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
let prompt_arg = match &command {
// Allow prompt before the subcommand by falling back to the parent-level prompt
// when the Resume subcommand did not provide its own prompt.
Some(ExecCommand::Resume(args)) => args.prompt.clone().or(prompt),
Some(ExecCommand::Resume(args)) => {
let resume_prompt = args
.prompt
.clone()
// When using `resume --last <PROMPT>`, 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
}
});
resume_prompt.or(prompt)
}
None => prompt,
};

Expand Down
54 changes: 54 additions & 0 deletions codex-rs/exec/tests/suite/resume.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,60 @@ fn exec_resume_last_appends_to_existing_file() -> anyhow::Result<()> {
Ok(())
}

#[test]
fn exec_resume_last_accepts_prompt_after_flag_in_json_mode() -> 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 with a unique marker in the content.
let marker = format!("resume-last-json-{}", 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();

// Find the created session file containing the marker.
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) Second run: resume the most recent file and pass the prompt after --last.
let marker2 = format!("resume-last-json-2-{}", Uuid::new_v4());
let prompt2 = format!("echo {marker2}");

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]
fn exec_resume_by_id_appends_to_existing_file() -> anyhow::Result<()> {
let test = test_codex_exec();
Expand Down
Loading