-
Notifications
You must be signed in to change notification settings - Fork 6.5k
codex-exec: allow resume --last to read prompt #6717 #6719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Dmitri Khokhlov <dkhokhlov@cribl.io>
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
| 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")] |
There was a problem hiding this comment.
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.
|
Thanks for the contribution. Please take a look at the codex code review above. |
|
@etraut-openai any action on my side? that code comment was from me. |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[arg(long = "last", default_value_t = false)] | ||
| pub last: bool, | ||
|
|
||
| /// Prompt to send after resuming the session. If `-` is used, read from stdin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍 / 👎.
There was a problem hiding this comment.
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.
Description
--last is set, so the flow now keeps working in JSON mode. (codex-rs/exec/src/cli.rs:84-104, codex-rs/exec/src/lib.rs:75-130)
Testing
#6717