[codex] Handle Ctrl-C for non-TTY unified exec#26734
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f765fdce24
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
f765fdc to
5f1f3e3
Compare
2ce392c to
ae2c317
Compare
ae2c317 to
b564da4
Compare
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b564da44fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review this |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
jif-oai
left a comment
There was a problem hiding this comment.
I guess you tried it? Should we add a small prompt in the tool def to encourage the model to use CTRL-C?
|
|
||
| pub(super) async fn interrupt(&self) -> Result<(), UnifiedExecError> { | ||
| match &self.process_handle { | ||
| ProcessHandle::Local(process_handle) => process_handle |
There was a problem hiding this comment.
(found by Codex)
This only signals the spawned zsh process group. With unified-exec zsh-fork, approved commands run under EscalateServer outside that group, and the super-exec path still does not forward signal
There was a problem hiding this comment.
I'll punt on zsh-fork for now.
| }, | ||
| ); | ||
| router.request( | ||
| EXEC_SIGNAL_METHOD, |
There was a problem hiding this comment.
Can we make signal out-of-band or dispatch post-handshake requests concurrently?
There was a problem hiding this comment.
I'd like to merge it as it. Signal has the same ordering semantic as write_stdin which we use for sigint in non-tty cases.
| ProcessSignal::Interrupt => { | ||
| #[cfg(unix)] | ||
| { | ||
| crate::process_group::interrupt_process_group(self.process_group_id) |
There was a problem hiding this comment.
This assumes SIGINT is default and unblocked in the child, but pipe spawn preserves the parent’s signal state. If Codex inherited SIGINT ignored, this returns success while the command keeps running
There was a problem hiding this comment.
I think this is fine. Model will recover from the fact the process is still running (by explicitly killing it like it does today)
| } | ||
| /// Send SIGINT to a specific process group ID (best-effort). | ||
| pub fn interrupt_process_group(process_group_id: u32) -> io::Result<()> { | ||
| signal_process_group_id(process_group_id as libc::pid_t, libc::SIGINT).map(|_| ()) |
There was a problem hiding this comment.
(found by Codex)
Can we cover the default SIGINT path too? The pipe waiter maps code() == None to -1, so ordinary Ctrl-C reports exitCode: -1
| ProcessSignal::Interrupt => { | ||
| #[cfg(unix)] | ||
| if let Some(process_group_id) = self.process_group_id { | ||
| return crate::process_group::interrupt_process_group(process_group_id); |
There was a problem hiding this comment.
I think that for PTYs, this cached group is the shell/session leader, not necessarily the terminal foreground group. But not sure and very nit anyway
It doesn't need to be encouraged. That's the default behavior by the model today. |
Why
A long-running unified exec process started with
tty: falsecould not be interrupted viawrite_stdin: ordinary non-TTY stdin writes are rejected once stdin is closed, but an exact U+0003 payload should still map to a process interrupt. The interrupt should flow through the same process lifecycle path as a real signal so Codex preserves process-reported output and exit metadata instead of fabricating a Ctrl-C exit code or tearing down the session early.What Changed
process/signalto exec-server withProcessSignal::Interruptand an empty response.ProcessHandle::signalpath for spawned processes; on Unix it sends SIGINT to the process group and leaves terminate/hard-kill unchanged.write_stdinthroughprocess.signal(...)instead ofterminate, then let the normal post-write collection path drain output and observe exit.trap INThandler prints the signal and exits with its own code.tty: falseprocess traps SIGINT, emits output, and exits with its own code.Validation
just test -p codex-exec-server exec_process_signal_interrupts_processjust test -p codex-exec-serverjust test -p codex-core write_stdin_ctrl_c_interrupts_non_tty_session