From 581909bbc43d36dd1968fb996f13b1f6156f22b6 Mon Sep 17 00:00:00 2001 From: SUN Yizhe Date: Wed, 28 Oct 2026 17:40:23 +0900 Subject: [PATCH] Fix compact cancellation and prepare v0.6.1 --- CHANGELOG.md | 6 ++ Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 16 +-- src/output.rs | 63 ++++++++++- src/process.rs | 35 ++++++- tests/cli.rs | 202 +++++++++++++++++++++++++++--------- tests/support/mock_codex.rs | 21 ++++ 8 files changed, 281 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d8aab8..3446c18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## 0.6.1 + +- Bound compact-mode cancellation to a 250 ms graceful shutdown window, then force-stop the owned process group; a second cancellation forces immediate shutdown while preserving the original signal exit status. +- Report turn-start activity immediately while preserving the final response on stdout. +- Label recoverable Codex diagnostics neutrally instead of presenting experimental-feature warnings as errors. + ## 0.6.0 - Add a public Rust library returning typed commit subjects and bodies from supplied public diffs. diff --git a/Cargo.lock b/Cargo.lock index 43db5cb..8f0b962 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -90,7 +90,7 @@ checksum = "1c133bc6a41be0d194c306b5506d15e6feeea7b1d6604bd3f8310dfb2ca96486" [[package]] name = "codex-commit" -version = "0.6.0" +version = "0.6.1" dependencies = [ "clap", "clap_complete", diff --git a/Cargo.toml b/Cargo.toml index b5643c4..afe849e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codex-commit" -version = "0.6.0" +version = "0.6.1" edition = "2024" rust-version = "1.85" description = "Commit Git changes with Codex, with optional push" diff --git a/README.md b/README.md index 43a0d34..bec86d0 100644 --- a/README.md +++ b/README.md @@ -39,18 +39,18 @@ Cargo compiles the published source for your machine and normally installs into ### Prebuilt macOS downloads -Download the matching archive and `SHA256SUMS` from [v0.6.0](https://github.com/syz51/codex-commit/releases/tag/v0.6.0): +Download the matching archive and `SHA256SUMS` from [v0.6.1](https://github.com/syz51/codex-commit/releases/tag/v0.6.1): | Mac | Archive | | --- | --- | -| Apple Silicon (M-series) | [`codex-commit-v0.6.0-aarch64-apple-darwin.tar.gz`](https://github.com/syz51/codex-commit/releases/download/v0.6.0/codex-commit-v0.6.0-aarch64-apple-darwin.tar.gz) | -| Intel | [`codex-commit-v0.6.0-x86_64-apple-darwin.tar.gz`](https://github.com/syz51/codex-commit/releases/download/v0.6.0/codex-commit-v0.6.0-x86_64-apple-darwin.tar.gz) | +| Apple Silicon (M-series) | [`codex-commit-v0.6.1-aarch64-apple-darwin.tar.gz`](https://github.com/syz51/codex-commit/releases/download/v0.6.1/codex-commit-v0.6.1-aarch64-apple-darwin.tar.gz) | +| Intel | [`codex-commit-v0.6.1-x86_64-apple-darwin.tar.gz`](https://github.com/syz51/codex-commit/releases/download/v0.6.1/codex-commit-v0.6.1-x86_64-apple-darwin.tar.gz) | For example, on Apple Silicon: ```sh -release=https://github.com/syz51/codex-commit/releases/download/v0.6.0 -archive=codex-commit-v0.6.0-aarch64-apple-darwin.tar.gz +release=https://github.com/syz51/codex-commit/releases/download/v0.6.1 +archive=codex-commit-v0.6.1-aarch64-apple-darwin.tar.gz curl -fLO "$release/$archive" curl -fLO "$release/SHA256SUMS" grep " $archive\$" SHA256SUMS | shasum -a 256 -c - @@ -136,7 +136,9 @@ The task asks Codex to inspect staged, unstaged, and untracked changes; follow r With `--push`, Codex can push existing local commits from a clean worktree. With no upstream, it may set one only when `origin` is the sole remote. Both push modes are prompt policies, not OS-enforced network or Git restrictions; review the streamed output. -Default unattended output shows concise progress on stderr and the final response on stdout. It renders Codex's JSON events, hiding the startup banner, echoed prompt, reasoning, routine hook notifications, and successful raw tool output. Failed command output and tool errors remain visible. The task asks for Git status and diff summaries, intended changes, checks, and the observed commit and push results without dumping full files or diffs. +Default unattended output immediately reports when a turn starts, shows concise progress on stderr, and keeps the final response on stdout. Codex JSON messages do not identify commentary separately, so the latest message is held until another message or tool activity establishes it as progress, or turn completion establishes it as the final response. Ctrl-C and other cancellation signals are forwarded immediately. If Codex has not exited after a 250 ms grace period, the supervisor force-stops its process group; a second cancellation forces shutdown immediately. Cancellation preserves the original signal exit status even when output is blocked. + +It renders Codex's JSON events, hiding the startup banner, echoed prompt, reasoning, routine hook notifications, and successful raw tool output. Failed command output and tool errors remain visible. The task asks for Git status and diff summaries, intended changes, checks, and the observed commit and push results without dumping full files or diffs. Use `--verbose` for Codex's full terminal output. Both `--verbose` and `--interactive` replace the Rust process with Codex on Unix, preserving its terminal streams and process identity. Compact mode supervises Codex instead, forwards interrupt and termination signals, and preserves its exit code; invalid or incomplete event streams fail explicitly. Piped or file input is passed to Codex as context; compact mode does not read from an interactive terminal. **Ctrl-C interrupts Codex** in all modes. Cancellation cannot undo an operation that already completed; inspect local and remote state before retrying. @@ -195,7 +197,7 @@ responsible for reviewing message accuracy and their public-data boundary. ```toml [dependencies] -codex-commit = "=0.6.0" +codex-commit = "=0.6.1" ``` ```rust,no_run diff --git a/src/output.rs b/src/output.rs index b4f6e1f..83de334 100644 --- a/src/output.rs +++ b/src/output.rs @@ -50,7 +50,10 @@ impl Renderer { "thread.started" => { writeln!(self.stderr, "Session: {}", string(&event, "thread_id")?)?; } - "turn.started" => self.completed = false, + "turn.started" => { + self.completed = false; + writeln!(self.stderr, "Codex is working (Ctrl-C to interrupt).")?; + } "turn.completed" => self.completed = true, "turn.failed" => { self.progress()?; @@ -65,7 +68,11 @@ impl Renderer { self.progress()?; // Diagnostics can precede a recovered turn. Completion and the // process status determine success; turn.failed remains decisive. - writeln!(self.stderr, "Codex error: {}", string(&event, "message")?)?; + writeln!( + self.stderr, + "Codex diagnostic: {}", + string(&event, "message")? + )?; } "item.started" | "item.updated" | "item.completed" => { let item = &event["item"]; @@ -82,7 +89,11 @@ impl Renderer { } "error" if completed => { self.progress()?; - writeln!(self.stderr, "Codex error: {}", string(item, "message")?)?; + writeln!( + self.stderr, + "Codex diagnostic: {}", + string(item, "message")? + )?; } "command_execution" => { let command = string(item, "command")?; @@ -197,6 +208,17 @@ mod tests { assert_eq!(String::from_utf8(err).unwrap(), "Inspecting changes\n"); } + #[test] + fn turn_start_shows_activity_before_any_message_or_tool() { + let mut renderer = Renderer::new(Vec::new(), Vec::new()); + renderer.event(r#"{"type":"turn.started"}"#).unwrap(); + assert_eq!( + renderer.stderr, + b"Codex is working (Ctrl-C to interrupt).\n" + ); + assert!(renderer.stdout.is_empty()); + } + #[test] fn successful_tool_output_and_reasoning_are_hidden() { let (success, out, err) = render(&[ @@ -276,8 +298,39 @@ mod tests { ]); assert!(success); assert_eq!(out, "Done\n"); - assert!(err.contains("retrying transport")); - assert!(err.contains("experimental feature warning")); + assert_eq!( + err, + "Codex diagnostic: retrying transport\nCodex diagnostic: experimental feature warning\n" + ); + } + + #[test] + fn chronicle_warning_remains_diagnostic_when_turn_succeeds_or_fails() { + let warning = "Under-development features enabled: chronicle. Under-development features are incomplete and may behave unpredictably. To suppress this warning, set `suppress_unstable_features_warning = true` in /home/user/.codex/config.toml."; + for diagnostic in [ + json!({"type":"error","message":warning}), + json!({"type":"item.completed","item":{"type":"error","message":warning}}), + ] { + let (success, out, err) = render(&[ + diagnostic.clone(), + message("Done"), + json!({"type":"turn.completed"}), + ]); + assert!(success); + assert_eq!(out, "Done\n"); + assert_eq!(err, format!("Codex diagnostic: {warning}\n")); + + let (success, out, err) = render(&[ + diagnostic, + json!({"type":"turn.failed","error":{"message":"model unavailable"}}), + ]); + assert!(!success); + assert!(out.is_empty()); + assert_eq!( + err, + format!("Codex diagnostic: {warning}\nCodex failed: model unavailable\n") + ); + } } #[test] diff --git a/src/process.rs b/src/process.rs index 37153dc..4c0313d 100644 --- a/src/process.rs +++ b/src/process.rs @@ -13,7 +13,9 @@ use std::process::{Command, Stdio}; use std::sync::atomic::{AtomicBool, AtomicI32, Ordering}; use std::sync::{Arc, Mutex}; use std::thread; -use std::time::Duration; +use std::time::{Duration, Instant}; + +const CANCELLATION_GRACE: Duration = Duration::from_millis(250); const MAX_EVENT: u64 = 16 * 1024 * 1024; @@ -35,6 +37,8 @@ pub(crate) fn run(mut command: Command) -> Result<(), CliError> { let forwarding = Arc::clone(&target); let cancellation = Arc::new(AtomicI32::new(0)); let forwarded_cancellation = Arc::clone(&cancellation); + let cancellation_deadline = Arc::new(Mutex::new(None)); + let forwarded_deadline = Arc::clone(&cancellation_deadline); let signal_thread = thread::spawn(move || { for signal in signals.forever() { let target = forwarding.lock().expect("signal target lock"); @@ -44,10 +48,28 @@ pub(crate) fn run(mut command: Command) -> Result<(), CliError> { signal, Signal::SIGINT | Signal::SIGTERM | Signal::SIGHUP | Signal::SIGQUIT ) { - forwarded_cancellation.store(signal as i32, Ordering::Release); + if forwarded_cancellation + .compare_exchange(0, signal as i32, Ordering::AcqRel, Ordering::Acquire) + .is_err() + { + // A second cancellation means stop now, preserving + // the first signal as the wrapper's exit status. + let _ = killpg(group, Signal::SIGKILL); + continue; + } + *forwarded_deadline + .lock() + .expect("cancellation deadline lock") = + Some(Instant::now() + CANCELLATION_GRACE); + let _ = killpg(group, signal); + // Pending cancellation must reach a stopped agent too. + let _ = killpg(group, Signal::SIGCONT); + continue; } let _ = killpg(group, signal); - if signal == Signal::SIGTSTP { + if signal == Signal::SIGTSTP + && forwarded_cancellation.load(Ordering::Acquire) == 0 + { // Stop the supervisor only after its owned group has // received suspension. SIGCONT will resume both. let _ = kill(getpid(), Signal::SIGSTOP); @@ -102,6 +124,13 @@ pub(crate) fn run(mut command: Command) -> Result<(), CliError> { loop { { let mut target = reaper_target.lock().expect("signal target lock"); + if cancellation_deadline + .lock() + .expect("cancellation deadline lock") + .is_some_and(|deadline| Instant::now() >= deadline) + { + let _ = killpg(group, Signal::SIGKILL); + } if let Some(status) = child.try_wait()? { let _ = killpg(group, Signal::SIGKILL); *target = None; diff --git a/tests/cli.rs b/tests/cli.rs index a79db8b..b2c4fbe 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1651,61 +1651,165 @@ fn compact_blocked_final_output_remains_interruptible_after_codex_exits() { #[test] fn compact_blocked_progress_cancellation_reaps_owned_processes() { - let fixture = Fixture::new(); - let (read, write) = nix::unistd::pipe().unwrap(); - let mut reader = File::from(read); - let flags = OFlag::from_bits_truncate(fcntl(&reader, FcntlArg::F_GETFL).unwrap()); - fcntl(&reader, FcntlArg::F_SETFL(flags | OFlag::O_NONBLOCK)).unwrap(); - let child = fixture - .command(cli()) - .env("FAKE_CODEX_MODE", "wait-child") - .env("FAKE_CODEX_LARGE_PROGRESS", "1") - .stdout(Stdio::null()) - .stderr(Stdio::from(write)) - .process_group(0) - .spawn() - .unwrap(); - let mut guard = CapturedChild { - child, - reaped: false, - }; - let deadline = Instant::now() + Duration::from_secs(5); - let mut output = Vec::new(); - loop { - let mut buffer = [0; 8192]; - match reader.read(&mut buffer) { - Ok(length) => output.extend_from_slice(&buffer[..length]), - Err(error) if error.kind() == io::ErrorKind::WouldBlock => {} - Err(error) => panic!("read stderr: {error}"), + for ignore_cancellation in [false, true] { + let mut fixture = Fixture::new(); + if ignore_cancellation { + fixture + .environment + .insert("FAKE_CODEX_IGNORE_CANCELLATION".into(), "1".into()); } - if text(&output).contains("BLOCKED_PROGRESS") { - break; + let (read, write) = nix::unistd::pipe().unwrap(); + let mut reader = File::from(read); + let flags = OFlag::from_bits_truncate(fcntl(&reader, FcntlArg::F_GETFL).unwrap()); + fcntl(&reader, FcntlArg::F_SETFL(flags | OFlag::O_NONBLOCK)).unwrap(); + let child = fixture + .command(cli()) + .env("FAKE_CODEX_MODE", "wait-child") + .env("FAKE_CODEX_LARGE_PROGRESS", "1") + .stdout(Stdio::null()) + .stderr(Stdio::from(write)) + .process_group(0) + .spawn() + .unwrap(); + let mut guard = CapturedChild { + child, + reaped: false, + }; + let deadline = Instant::now() + Duration::from_secs(5); + let mut output = Vec::new(); + loop { + let mut buffer = [0; 8192]; + match reader.read(&mut buffer) { + Ok(length) => output.extend_from_slice(&buffer[..length]), + Err(error) if error.kind() == io::ErrorKind::WouldBlock => {} + Err(error) => panic!("read stderr: {error}"), + } + if text(&output).contains("BLOCKED_PROGRESS") { + break; + } + assert!(Instant::now() < deadline); + std::thread::sleep(Duration::from_millis(10)); } - assert!(Instant::now() < deadline); - std::thread::sleep(Duration::from_millis(10)); + let codex = fixture.invocation().pid; + struct AgentCleanup(Pid); + impl Drop for AgentCleanup { + fn drop(&mut self) { + let _ = killpg(self.0, Signal::SIGKILL); + } + } + let _agent_cleanup = AgentCleanup(Pid::from_raw(codex as i32)); + let descendant: Value = + serde_json::from_slice(&fs::read(&fixture.child_log).unwrap()).unwrap(); + assert!(guard.child.try_wait().unwrap().is_none()); + kill(Pid::from_raw(guard.child.id() as i32), Signal::SIGTERM).unwrap(); + let status = loop { + if let Some(status) = guard.child.try_wait().unwrap() { + break status; + } + assert!( + Instant::now() < deadline, + "first TERM must interrupt blocked progress" + ); + std::thread::sleep(Duration::from_millis(10)); + }; + guard.reaped = true; + assert_eq!(shell_status(status), 143); + assert_eq!(kill(Pid::from_raw(codex as i32), None), Err(Errno::ESRCH)); + assert_eq!( + kill( + Pid::from_raw(descendant["pid"].as_i64().unwrap() as i32), + None + ), + Err(Errno::ESRCH) + ); } - let codex = fixture.invocation().pid; - let descendant: Value = serde_json::from_slice(&fs::read(&fixture.child_log).unwrap()).unwrap(); - assert!(guard.child.try_wait().unwrap().is_none()); - kill(Pid::from_raw(guard.child.id() as i32), Signal::SIGTERM).unwrap(); - let status = loop { - if let Some(status) = guard.child.try_wait().unwrap() { - break status; +} + +#[test] +fn compact_cancellation_bounds_unresponsive_codex_shutdown() { + for signal in [ + Signal::SIGINT, + Signal::SIGTERM, + Signal::SIGHUP, + Signal::SIGQUIT, + ] { + let mut fixture = Fixture::new(); + fixture + .environment + .insert("FAKE_CODEX_IGNORE_CANCELLATION".into(), "1".into()); + let mut terminal = Terminal::launch_mode(&fixture, false, true); + terminal.wait_for_output(false); + let codex = Pid::from_raw(fixture.invocation().pid as i32); + let start = Instant::now(); + // Always clean the separate agent group, even when testing old code. + let result = + std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| terminal.cancel(signal))); + let _ = killpg(codex, Signal::SIGKILL); + let status = result.expect("unresponsive Codex must be forcibly cancelled"); + assert_eq!(shell_status(status), 128 + signal as i32); + assert!(start.elapsed() < Duration::from_secs(2)); + assert_eq!(kill(codex, None), Err(Errno::ESRCH)); + } +} + +#[test] +fn compact_reports_activity_without_waiting_for_a_message() { + let mut fixture = Fixture::new(); + fixture + .environment + .insert("FAKE_CODEX_IDLE_WITHOUT_MESSAGE".into(), "1".into()); + let mut terminal = Terminal::launch_mode(&fixture, false, true); + let deadline = Instant::now() + Duration::from_secs(5); + loop { + terminal.drain(); + if text(&terminal.output).contains("Codex is working (Ctrl-C to interrupt).") { + break; } assert!( Instant::now() < deadline, - "first TERM must interrupt blocked progress" + "turn start must be visible before any message" ); std::thread::sleep(Duration::from_millis(10)); - }; - guard.reaped = true; - assert_eq!(shell_status(status), 143); - assert_eq!(kill(Pid::from_raw(codex as i32), None), Err(Errno::ESRCH)); - assert_eq!( - kill( - Pid::from_raw(descendant["pid"].as_i64().unwrap() as i32), - None - ), - Err(Errno::ESRCH) - ); + } + assert!(terminal.child.try_wait().unwrap().is_none()); + assert!(!text(&terminal.output).contains("live output")); + assert_eq!(shell_status(terminal.cancel(Signal::SIGINT)), 130); +} + +#[test] +fn compact_cancels_stopped_agent_and_preserves_first_signal_on_repeat() { + for stopped in [true, false] { + let mut fixture = Fixture::new(); + fixture + .environment + .insert("FAKE_CODEX_IGNORE_CANCELLATION".into(), "1".into()); + let mut terminal = Terminal::launch_mode(&fixture, false, true); + terminal.wait_for_output(false); + let codex = Pid::from_raw(fixture.invocation().pid as i32); + if stopped { + killpg(codex, Signal::SIGSTOP).unwrap(); + } + terminal.master.write_all(&[3]).unwrap(); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + if stopped { + // The pending INT reaches the resumed agent; its ignored + // disposition must still lead to deadline escalation. + let deadline = Instant::now() + Duration::from_secs(2); + loop { + if let Some(status) = terminal.child.try_wait().unwrap() { + terminal.finished = true; + break status; + } + assert!(Instant::now() < deadline); + std::thread::sleep(Duration::from_millis(10)); + } + } else { + std::thread::sleep(Duration::from_millis(30)); + terminal.cancel(Signal::SIGTERM) + } + })); + let _ = killpg(codex, Signal::SIGKILL); + assert_eq!(shell_status(result.unwrap()), 130); + assert_eq!(kill(codex, None), Err(Errno::ESRCH)); + } } diff --git a/tests/support/mock_codex.rs b/tests/support/mock_codex.rs index 2b893b1..997e3de 100644 --- a/tests/support/mock_codex.rs +++ b/tests/support/mock_codex.rs @@ -85,6 +85,21 @@ fn main() -> Result<(), Box> { sigaction(Signal::SIGTERM, &action)?; } + if env::var_os("FAKE_CODEX_IGNORE_CANCELLATION").is_some() { + let ignore = SigAction::new(SigHandler::SigIgn, SaFlags::empty(), SigSet::empty()); + // Deliberately model an agent that cannot gracefully shut down. + for signal in [ + Signal::SIGINT, + Signal::SIGTERM, + Signal::SIGHUP, + Signal::SIGQUIT, + ] { + unsafe { + sigaction(signal, &ignore)?; + } + } + } + let args: Vec<_> = env::args_os().skip(1).collect(); let argument_bytes: Vec<_> = args.iter().map(|arg| arg.as_bytes()).collect(); fs::write( @@ -103,6 +118,12 @@ fn main() -> Result<(), Box> { ); println!("{}", json!({"type":"turn.started"})); } + if env::var_os("FAKE_CODEX_IDLE_WITHOUT_MESSAGE").is_some() { + io::stdout().flush()?; + loop { + std::thread::sleep(Duration::from_millis(100)); + } + } let mode = env::var("FAKE_CODEX_MODE").unwrap_or_default(); match mode.as_str() { "stream" | "wait" | "wait-child" => {