From 7ecd3153a8e89834ab6237cad601fad265f6c2c8 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Mon, 28 Jul 2025 07:45:49 -0700 Subject: [PATCH 1/6] fix: correctly wrap history items (#1685) The overall idea here is: skip ratatui for writing into scrollback, because its primitives are wrong. We want to render full lines of text, that will be wrapped natively by the terminal, and which we never plan to update using ratatui (so the `Buffer` struct is overhead and in fact an inhibition). Instead, we use ANSI scrolling regions (link reference doc to come). Essentially, we: 1. Define a scrolling region that extends from the top of the prompt area all the way to the top of scrollback 2. Scroll that region up by N < (screen_height - viewport_height) lines, in this PR N=1 3. Put our cursor at the top of the newly empty region 4. Print out our new text like normal The terminal interactions here (write_spans and its dependencies) are mostly extracted from ratatui. --- codex-rs/Cargo.lock | 89 ++++---- codex-rs/Cargo.toml | 5 + codex-rs/tui/Cargo.toml | 3 +- codex-rs/tui/src/insert_history.rs | 320 ++++++++++++++--------------- 4 files changed, 208 insertions(+), 209 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index ba71596ecd..da3bd50a85 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -463,18 +463,18 @@ checksum = "df8670b8c7b9dae1793364eafadf7239c40d669904660c5960d74cfd80b46a53" [[package]] name = "castaway" -version = "0.2.3" +version = "0.2.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0abae9be0aaf9ea96a3b1b8b1b55c602ca751eba1b1500220cea4ecbafe7c0d5" +checksum = "dec551ab6e7578819132c713a93c022a05d60159dc86e7a7050223577484c55a" dependencies = [ "rustversion", ] [[package]] name = "cc" -version = "1.2.29" +version = "1.2.30" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5c1599538de2394445747c8cf7935946e3cc27e9625f889d979bfb2aaf569362" +checksum = "deec109607ca693028562ed836a5f1c4b8bd77755c4e132fc5ce11b0b6211ae7" dependencies = [ "jobserver", "libc", @@ -570,9 +570,9 @@ checksum = "b94f61472cee1439c0b966b47e3aca9ae07e45d070759512cd390ea2bebc6675" [[package]] name = "clipboard-win" -version = "5.4.0" +version = "5.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "15efe7a882b08f34e38556b14f2fb3daa98769d06c7f0c1b076dfd0d983bc892" +checksum = "bde03770d3df201d4fb868f2c9c59e66a3e4e2bd06692a0fe701e7103c7e84d4" dependencies = [ "error-code", ] @@ -978,9 +978,9 @@ dependencies = [ [[package]] name = "crc32fast" -version = "1.4.2" +version = "1.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a97769d94ddab943e4510d138150169a2758b5ef3eb191a9ee688de3e23ef7b3" +checksum = "9481c1c90cbf2ac953f07c8d4a58aa3945c425b7185c9154d67a65e4230da511" dependencies = [ "cfg-if", ] @@ -1527,7 +1527,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0ce92ff622d6dadf7349484f42c93271a0d49b7cc4d466a936405bacbe10aa78" dependencies = [ "cfg-if", - "rustix 1.0.7", + "rustix 1.0.8", "windows-sys 0.59.0", ] @@ -1976,9 +1976,9 @@ dependencies = [ [[package]] name = "hyper-util" -version = "0.1.15" +version = "0.1.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7f66d5bd4c6f02bf0542fad85d626775bab9258cf795a4256dcaf3161114d1df" +checksum = "8d9b05277c7e8da2c93a568989bb6207bef0112e8d17df7a6eda4a3cf143bc5e" dependencies = [ "base64 0.22.1", "bytes", @@ -1992,7 +1992,7 @@ dependencies = [ "libc", "percent-encoding", "pin-project-lite", - "socket2", + "socket2 0.6.0", "system-configuration", "tokio", "tower-service", @@ -2245,9 +2245,9 @@ dependencies = [ [[package]] name = "instability" -version = "0.3.7" +version = "0.3.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0bf9fed6d91cfb734e7476a06bde8300a1b94e217e1b523b6f0cd1a01998c71d" +checksum = "435d80800b936787d62688c927b6490e887c7ef5ff9ce922c6c6050fca75eb9a" dependencies = [ "darling", "indoc", @@ -2278,9 +2278,9 @@ dependencies = [ [[package]] name = "io-uring" -version = "0.7.8" +version = "0.7.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b86e202f00093dcba4275d4636b93ef9dd75d025ae560d2521b45ea28ab49013" +checksum = "d93587f37623a1a17d94ef2bc9ada592f5465fe7732084ab7beefabe5c77c0c4" dependencies = [ "bitflags 2.9.1", "cfg-if", @@ -2484,9 +2484,9 @@ dependencies = [ [[package]] name = "libredox" -version = "0.1.4" +version = "0.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1580801010e535496706ba011c15f8532df6b42297d2e471fec38ceadd8c0638" +checksum = "4488594b9328dee448adb906d8b126d9b7deb7cf5c22161ee591610bb1be83c0" dependencies = [ "bitflags 2.9.1", "libc", @@ -3359,8 +3359,7 @@ dependencies = [ [[package]] name = "ratatui" version = "0.29.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eabd94c2f37801c20583fc49dd5cd6b0ba68c716787c2dd6ed18571e1e63117b" +source = "git+https://github.com/nornagon/ratatui?branch=nornagon-v0.29.0-patch#bca287ddc5d38fe088c79e2eda22422b96226f2e" dependencies = [ "bitflags 2.9.1", "cassowary", @@ -3465,9 +3464,9 @@ dependencies = [ [[package]] name = "redox_syscall" -version = "0.5.13" +version = "0.5.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0d04b7d0ee6b4a0207a0a7adb104d23ecb0b47d6beae7152d0fa34b692b29fd6" +checksum = "7e8af0dde094006011e6a740d4879319439489813bd0bcdc7d821beaeeff48ec" dependencies = [ "bitflags 2.9.1", ] @@ -3615,9 +3614,9 @@ dependencies = [ [[package]] name = "rgb" -version = "0.8.51" +version = "0.8.52" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a457e416a0f90d246a4c3288bd7a25b2304ca727f253f95be383dd17af56be8f" +checksum = "0c6a884d2998352bb4daf0183589aec883f16a6da1f4dde84d8e2e9a5409a1ce" [[package]] name = "ring" @@ -3693,22 +3692,22 @@ dependencies = [ [[package]] name = "rustix" -version = "1.0.7" +version = "1.0.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c71e83d6afe7ff64890ec6b71d6a69bb8a610ab78ce364b3352876bb4c801266" +checksum = "11181fbabf243db407ef8df94a6ce0b2f9a733bd8be4ad02b4eda9602296cac8" dependencies = [ "bitflags 2.9.1", "errno", "libc", "linux-raw-sys 0.9.4", - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] name = "rustls" -version = "0.23.28" +version = "0.23.29" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7160e3e10bf4535308537f3c4e1641468cd0e485175d6163087c0393c7d46643" +checksum = "2491382039b29b9b11ff08b76ff6c97cf287671dbb74f0be44bda389fffe9bd1" dependencies = [ "once_cell", "rustls-pki-types", @@ -3728,9 +3727,9 @@ dependencies = [ [[package]] name = "rustls-webpki" -version = "0.103.3" +version = "0.103.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e4a72fe2bcf7a6ac6fd7d0b9e5cb68aeb7d4c0a0271730218b3e92d43b4eb435" +checksum = "0a17884ae0c1b773f1ccd2bd4a8c72f16da897310a98b0e84bf349ad5ead92fc" dependencies = [ "ring", "rustls-pki-types", @@ -3956,9 +3955,9 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.140" +version = "1.0.141" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "20068b6e96dc6c9bd23e01df8827e6c7e1f2fddd43c21810382803c136b99373" +checksum = "30b9eff21ebe718216c6ec64e1d9ac57087aad11efc64e32002bce4a0d4c03d3" dependencies = [ "indexmap 2.10.0", "itoa", @@ -4151,6 +4150,16 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "socket2" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "233504af464074f9d066d7b5416c5f9b894a5862a6506e306f7b816cdd6f1807" +dependencies = [ + "libc", + "windows-sys 0.59.0", +] + [[package]] name = "stable_deref_trait" version = "1.2.0" @@ -4442,7 +4451,7 @@ dependencies = [ "fastrand", "getrandom 0.3.3", "once_cell", - "rustix 1.0.7", + "rustix 1.0.8", "windows-sys 0.59.0", ] @@ -4463,7 +4472,7 @@ version = "0.4.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "45c6481c4829e4cc63825e62c49186a34538b7b2750b73b266581ffb612fb5ed" dependencies = [ - "rustix 1.0.7", + "rustix 1.0.8", "windows-sys 0.59.0", ] @@ -4609,7 +4618,7 @@ dependencies = [ "pin-project-lite", "signal-hook-registry", "slab", - "socket2", + "socket2 0.5.10", "tokio-macros", "windows-sys 0.52.0", ] @@ -4751,9 +4760,9 @@ dependencies = [ [[package]] name = "toml_writer" -version = "1.0.0" +version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b679217f2848de74cabd3e8fc5e6d66f40b7da40f8e1954d92054d9010690fd5" +checksum = "fcc842091f2def52017664b53082ecbbeb5c7731092bad69d2c63050401dfd64" [[package]] name = "tower" @@ -5575,9 +5584,9 @@ checksum = "271414315aff87387382ec3d271b52d7ae78726f5d44ac98b4f4030c91880486" [[package]] name = "winnow" -version = "0.7.11" +version = "0.7.12" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "74c7b26e3480b707944fc872477815d29a8e429d2f93a1ce000f5fa84a15cbcd" +checksum = "f3edebf492c8125044983378ecb5766203ad3b4c2f7a922bd7dd207f6d443e95" dependencies = [ "memchr", ] diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index eba43e548b..6f89e8faa7 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -40,3 +40,8 @@ strip = "symbols" # See https://github.com/openai/codex/issues/1411 for details. codegen-units = 1 + +[patch.crates-io] +# ratatui = { path = "../../ratatui" } +ratatui = { git = "https://github.com/nornagon/ratatui", branch = "nornagon-v0.29.0-patch" } + diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 9d73e3b386..b88ac8a080 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -35,8 +35,9 @@ lazy_static = "1" mcp-types = { path = "../mcp-types" } path-clean = "1.0.1" ratatui = { version = "0.29.0", features = [ - "unstable-widget-ref", + "scrolling-regions", "unstable-rendered-line-info", + "unstable-widget-ref", ] } ratatui-image = "8.0.0" regex-lite = "0.1" diff --git a/codex-rs/tui/src/insert_history.rs b/codex-rs/tui/src/insert_history.rs index 247e024cb0..7948436cd8 100644 --- a/codex-rs/tui/src/insert_history.rs +++ b/codex-rs/tui/src/insert_history.rs @@ -1,178 +1,162 @@ +use std::io; +use std::io::Write; + use crate::tui; -use ratatui::layout::Rect; -use ratatui::style::Style; +use crossterm::queue; +use crossterm::style::Color as CColor; +use crossterm::style::Colors; +use crossterm::style::Print; +use crossterm::style::SetAttribute; +use crossterm::style::SetBackgroundColor; +use crossterm::style::SetColors; +use crossterm::style::SetForegroundColor; +use ratatui::layout::Position; +use ratatui::prelude::Backend; +use ratatui::style::Color; +use ratatui::style::Modifier; use ratatui::text::Line; use ratatui::text::Span; -use ratatui::widgets::Paragraph; -use ratatui::widgets::Widget; -use unicode_width::UnicodeWidthChar; - -/// Insert a batch of history lines into the terminal scrollback above the -/// inline viewport. -/// -/// The incoming `lines` are the logical lines supplied by the -/// `ConversationHistory`. They may contain embedded newlines and arbitrary -/// runs of whitespace inside individual [`Span`]s. All of that must be -/// normalised before writing to the backing terminal buffer because the -/// ratatui [`Paragraph`] widget does not perform soft‑wrapping when used in -/// conjunction with [`Terminal::insert_before`]. -/// -/// This function performs a minimal wrapping / normalisation pass: -/// -/// * A terminal width is determined via `Terminal::size()` (falling back to -/// 80 columns if the size probe fails). -/// * Each logical line is broken into words and whitespace. Consecutive -/// whitespace is collapsed to a single space; leading whitespace is -/// discarded. -/// * Words that do not fit on the current line cause a soft wrap. Extremely -/// long words (longer than the terminal width) are split character by -/// character so they still populate the display instead of overflowing the -/// line. -/// * Explicit `\n` characters inside a span force a hard line break. -/// * Empty lines (including a trailing newline at the end of the batch) are -/// preserved so vertical spacing remains faithful to the logical history. -/// -/// Finally the physical lines are rendered directly into the terminal's -/// scrollback region using [`Terminal::insert_before`]. Any backend error is -/// ignored: failing to insert history is non‑fatal and a subsequent redraw -/// will eventually repaint a consistent view. -fn display_width(s: &str) -> usize { - s.chars() - .map(|c| UnicodeWidthChar::width(c).unwrap_or(0)) - .sum() -} - -struct LineBuilder { - term_width: usize, - spans: Vec>, - width: usize, -} - -impl LineBuilder { - fn new(term_width: usize) -> Self { - Self { - term_width, - spans: Vec::new(), - width: 0, - } - } - - fn flush_line(&mut self, out: &mut Vec>) { - out.push(Line::from(std::mem::take(&mut self.spans))); - self.width = 0; - } - - fn push_segment(&mut self, text: String, style: Style) { - self.width += display_width(&text); - self.spans.push(Span::styled(text, style)); - } - - fn push_word(&mut self, word: &mut String, style: Style, out: &mut Vec>) { - if word.is_empty() { - return; - } - let w_len = display_width(word); - if self.width > 0 && self.width + w_len > self.term_width { - self.flush_line(out); - } - if w_len > self.term_width && self.width == 0 { - // Split an overlong word across multiple lines. - let mut cur = String::new(); - let mut cur_w = 0; - for ch in word.chars() { - let ch_w = UnicodeWidthChar::width(ch).unwrap_or(0); - if cur_w + ch_w > self.term_width && cur_w > 0 { - self.push_segment(cur.clone(), style); - self.flush_line(out); - cur.clear(); - cur_w = 0; - } - cur.push(ch); - cur_w += ch_w; - } - if !cur.is_empty() { - self.push_segment(cur, style); - } - } else { - self.push_segment(word.clone(), style); - } - word.clear(); - } - - fn consume_whitespace(&mut self, ws: &mut String, style: Style, out: &mut Vec>) { - if ws.is_empty() { - return; - } - let space_w = display_width(ws); - if self.width > 0 && self.width + space_w > self.term_width { - self.flush_line(out); - } - if self.width > 0 { - self.push_segment(" ".to_string(), style); - } - ws.clear(); - } -} pub(crate) fn insert_history_lines(terminal: &mut tui::Tui, lines: Vec>) { - let term_width = terminal.size().map(|a| a.width).unwrap_or(80) as usize; - let mut physical: Vec> = Vec::new(); - - for logical in lines.into_iter() { - if logical.spans.is_empty() { - physical.push(logical); - continue; - } - - let mut builder = LineBuilder::new(term_width); - let mut buf_space = String::new(); - - for span in logical.spans.into_iter() { - let style = span.style; - let mut buf_word = String::new(); - - for ch in span.content.chars() { - if ch == '\n' { - builder.push_word(&mut buf_word, style, &mut physical); - buf_space.clear(); - builder.flush_line(&mut physical); - continue; - } - if ch.is_whitespace() { - builder.push_word(&mut buf_word, style, &mut physical); - buf_space.push(ch); - } else { - builder.consume_whitespace(&mut buf_space, style, &mut physical); - buf_word.push(ch); - } - if builder.width >= term_width { - builder.flush_line(&mut physical); - } - } - builder.push_word(&mut buf_word, style, &mut physical); - // whitespace intentionally left to allow collapsing across spans - } - if !builder.spans.is_empty() { - physical.push(Line::from(std::mem::take(&mut builder.spans))); + let screen_height = terminal + .backend() + .size() + .map(|s| s.height) + .unwrap_or(0xffffu16); + let mut area = terminal.get_frame().area(); + // We scroll up one line at a time because we can't position the cursor + // above the top of the screen. i.e. if + // lines.len() > screen_height - area.top() + // we would need to print the first line above the top of the screen, which + // can't be done. + for line in lines.into_iter() { + // 1. Scroll everything above the viewport up by one line + if area.bottom() >= screen_height { + let top = area.top(); + terminal.backend_mut().scroll_region_up(0..top, 1).ok(); + // 2. Move the cursor to the blank line + terminal.set_cursor_position(Position::new(0, top - 1)).ok(); } else { - // Preserve explicit blank line (e.g. due to a trailing newline). - physical.push(Line::from(Vec::>::new())); + // If the viewport isn't at the bottom of the screen, scroll down instead + terminal + .backend_mut() + .scroll_region_down(area.top()..area.bottom() + 1, 1) + .ok(); + terminal + .set_cursor_position(Position::new(0, area.top())) + .ok(); + area.y += 1; } + // 3. Write the line + write_spans(&mut std::io::stdout(), line.iter()).ok(); + } + terminal.set_viewport_area(area); +} + +struct ModifierDiff { + pub from: Modifier, + pub to: Modifier, +} + +impl ModifierDiff { + fn queue(self, mut w: W) -> io::Result<()> + where + W: io::Write, + { + use crossterm::style::Attribute as CAttribute; + let removed = self.from - self.to; + if removed.contains(Modifier::REVERSED) { + queue!(w, SetAttribute(CAttribute::NoReverse))?; + } + if removed.contains(Modifier::BOLD) { + queue!(w, SetAttribute(CAttribute::NormalIntensity))?; + if self.to.contains(Modifier::DIM) { + queue!(w, SetAttribute(CAttribute::Dim))?; + } + } + if removed.contains(Modifier::ITALIC) { + queue!(w, SetAttribute(CAttribute::NoItalic))?; + } + if removed.contains(Modifier::UNDERLINED) { + queue!(w, SetAttribute(CAttribute::NoUnderline))?; + } + if removed.contains(Modifier::DIM) { + queue!(w, SetAttribute(CAttribute::NormalIntensity))?; + } + if removed.contains(Modifier::CROSSED_OUT) { + queue!(w, SetAttribute(CAttribute::NotCrossedOut))?; + } + if removed.contains(Modifier::SLOW_BLINK) || removed.contains(Modifier::RAPID_BLINK) { + queue!(w, SetAttribute(CAttribute::NoBlink))?; + } + + let added = self.to - self.from; + if added.contains(Modifier::REVERSED) { + queue!(w, SetAttribute(CAttribute::Reverse))?; + } + if added.contains(Modifier::BOLD) { + queue!(w, SetAttribute(CAttribute::Bold))?; + } + if added.contains(Modifier::ITALIC) { + queue!(w, SetAttribute(CAttribute::Italic))?; + } + if added.contains(Modifier::UNDERLINED) { + queue!(w, SetAttribute(CAttribute::Underlined))?; + } + if added.contains(Modifier::DIM) { + queue!(w, SetAttribute(CAttribute::Dim))?; + } + if added.contains(Modifier::CROSSED_OUT) { + queue!(w, SetAttribute(CAttribute::CrossedOut))?; + } + if added.contains(Modifier::SLOW_BLINK) { + queue!(w, SetAttribute(CAttribute::SlowBlink))?; + } + if added.contains(Modifier::RAPID_BLINK) { + queue!(w, SetAttribute(CAttribute::RapidBlink))?; + } + + Ok(()) + } +} + +fn write_spans<'a, I>(mut writer: &mut impl Write, content: I) -> io::Result<()> +where + I: Iterator>, +{ + let mut fg = Color::Reset; + let mut bg = Color::Reset; + let mut modifier = Modifier::empty(); + for span in content { + let mut next_modifier = modifier; + next_modifier.insert(span.style.add_modifier); + next_modifier.remove(span.style.sub_modifier); + if next_modifier != modifier { + let diff = ModifierDiff { + from: modifier, + to: next_modifier, + }; + diff.queue(&mut writer)?; + modifier = next_modifier; + } + let next_fg = span.style.fg.unwrap_or(Color::Reset); + let next_bg = span.style.bg.unwrap_or(Color::Reset); + if next_fg != fg || next_bg != bg { + queue!( + writer, + SetColors(Colors::new(next_fg.into(), next_bg.into())) + )?; + fg = next_fg; + bg = next_bg; + } + + queue!(writer, Print(span.content.clone()))?; } - let total = physical.len() as u16; - terminal - .insert_before(total, |buf| { - let width = buf.area.width; - for (i, line) in physical.into_iter().enumerate() { - let area = Rect { - x: 0, - y: i as u16, - width, - height: 1, - }; - Paragraph::new(line).render(area, buf); - } - }) - .ok(); + queue!( + writer, + SetForegroundColor(CColor::Reset), + SetBackgroundColor(CColor::Reset), + SetAttribute(crossterm::style::Attribute::Reset), + ) } From 9102255854eab4310d7d90ab64215bf3f2e06850 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Jul 2025 08:31:24 -0700 Subject: [PATCH 2/6] fix: move arg0 handling out of codex-linux-sandbox and into its own crate (#1697) --- codex-rs/Cargo.lock | 20 ++++++--- codex-rs/Cargo.toml | 1 + codex-rs/arg0/Cargo.toml | 18 ++++++++ codex-rs/arg0/src/lib.rs | 68 +++++++++++++++++++++++++++++++ codex-rs/cli/Cargo.toml | 2 +- codex-rs/cli/src/main.rs | 3 +- codex-rs/exec/Cargo.toml | 2 +- codex-rs/exec/src/main.rs | 3 +- codex-rs/linux-sandbox/Cargo.toml | 14 +++---- codex-rs/linux-sandbox/src/lib.rs | 65 +---------------------------- codex-rs/mcp-server/Cargo.toml | 2 +- codex-rs/mcp-server/src/main.rs | 3 +- codex-rs/tui/Cargo.toml | 2 +- codex-rs/tui/src/main.rs | 3 +- 14 files changed, 121 insertions(+), 85 deletions(-) create mode 100644 codex-rs/arg0/Cargo.toml create mode 100644 codex-rs/arg0/src/lib.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index da3bd50a85..f3903d6ea6 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -605,6 +605,17 @@ dependencies = [ "tree-sitter-bash", ] +[[package]] +name = "codex-arg0" +version = "0.0.0" +dependencies = [ + "anyhow", + "codex-core", + "codex-linux-sandbox", + "dotenvy", + "tokio", +] + [[package]] name = "codex-chatgpt" version = "0.0.0" @@ -628,11 +639,11 @@ dependencies = [ "anyhow", "clap", "clap_complete", + "codex-arg0", "codex-chatgpt", "codex-common", "codex-core", "codex-exec", - "codex-linux-sandbox", "codex-login", "codex-mcp-server", "codex-tui", @@ -709,9 +720,9 @@ dependencies = [ "anyhow", "chrono", "clap", + "codex-arg0", "codex-common", "codex-core", - "codex-linux-sandbox", "owo-colors", "serde_json", "shlex", @@ -761,7 +772,6 @@ dependencies = [ "clap", "codex-common", "codex-core", - "dotenvy", "landlock", "libc", "seccompiler", @@ -799,8 +809,8 @@ version = "0.0.0" dependencies = [ "anyhow", "assert_cmd", + "codex-arg0", "codex-core", - "codex-linux-sandbox", "mcp-types", "mcp_test_support", "pretty_assertions", @@ -826,10 +836,10 @@ dependencies = [ "base64 0.22.1", "clap", "codex-ansi-escape", + "codex-arg0", "codex-common", "codex-core", "codex-file-search", - "codex-linux-sandbox", "codex-login", "color-eyre", "crossterm", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 6f89e8faa7..51b2b5cc12 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -3,6 +3,7 @@ resolver = "2" members = [ "ansi-escape", "apply-patch", + "arg0", "cli", "common", "core", diff --git a/codex-rs/arg0/Cargo.toml b/codex-rs/arg0/Cargo.toml new file mode 100644 index 0000000000..9ad1896746 --- /dev/null +++ b/codex-rs/arg0/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "codex-arg0" +version = { workspace = true } +edition = "2024" + +[lib] +name = "codex_arg0" +path = "src/lib.rs" + +[lints] +workspace = true + +[dependencies] +anyhow = "1" +codex-core = { path = "../core" } +codex-linux-sandbox = { path = "../linux-sandbox" } +dotenvy = "0.15.7" +tokio = { version = "1", features = ["rt-multi-thread"] } diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs new file mode 100644 index 0000000000..86b98c7d30 --- /dev/null +++ b/codex-rs/arg0/src/lib.rs @@ -0,0 +1,68 @@ +use std::future::Future; +use std::path::Path; +use std::path::PathBuf; + +/// While we want to deploy the Codex CLI as a single executable for simplicity, +/// we also want to expose some of its functionality as distinct CLIs, so we use +/// the "arg0 trick" to determine which CLI to dispatch. This effectively allows +/// us to simulate deploying multiple executables as a single binary on Mac and +/// Linux (but not Windows). +/// +/// When the current executable is invoked through the hard-link or alias named +/// `codex-linux-sandbox` we *directly* execute +/// [`codex_linux_sandbox::run_main`] (which never returns). Otherwise we: +/// +/// 1. Use [`dotenvy::from_path`] and [`dotenvy::dotenv`] to modify the +/// environment before creating any threads. +/// 2. Construct a Tokio multi-thread runtime. +/// 3. Derive the path to the current executable (so children can re-invoke the +/// sandbox) when running on Linux. +/// 4. Execute the provided async `main_fn` inside that runtime, forwarding any +/// error. Note that `main_fn` receives `codex_linux_sandbox_exe: +/// Option`, as an argument, which is generally needed as part of +/// constructing [`codex_core::config::Config`]. +/// +/// This function should be used to wrap any `main()` function in binary crates +/// in this workspace that depends on these helper CLIs. +pub fn arg0_dispatch_or_else(main_fn: F) -> anyhow::Result<()> +where + F: FnOnce(Option) -> Fut, + Fut: Future>, +{ + // Determine if we were invoked via the special alias. + let argv0 = std::env::args().next().unwrap_or_default(); + let exe_name = Path::new(&argv0) + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or(""); + + if exe_name == "codex-linux-sandbox" { + // Safety: [`run_main`] never returns. + codex_linux_sandbox::run_main(); + } + + // This modifies the environment, which is not thread-safe, so do this + // before creating any threads/the Tokio runtime. + load_dotenv(); + + // Regular invocation – create a Tokio runtime and execute the provided + // async entry-point. + let runtime = tokio::runtime::Runtime::new()?; + runtime.block_on(async move { + let codex_linux_sandbox_exe: Option = if cfg!(target_os = "linux") { + std::env::current_exe().ok() + } else { + None + }; + + main_fn(codex_linux_sandbox_exe).await + }) +} + +/// Load env vars from ~/.codex/.env and `$(pwd)/.env`. +fn load_dotenv() { + if let Ok(codex_home) = codex_core::config::find_codex_home() { + dotenvy::from_path(codex_home.join(".env")).ok(); + } + dotenvy::dotenv().ok(); +} diff --git a/codex-rs/cli/Cargo.toml b/codex-rs/cli/Cargo.toml index 943788157b..ab98764bed 100644 --- a/codex-rs/cli/Cargo.toml +++ b/codex-rs/cli/Cargo.toml @@ -18,12 +18,12 @@ workspace = true anyhow = "1" clap = { version = "4", features = ["derive"] } clap_complete = "4" +codex-arg0 = { path = "../arg0" } codex-chatgpt = { path = "../chatgpt" } codex-core = { path = "../core" } codex-common = { path = "../common", features = ["cli"] } codex-exec = { path = "../exec" } codex-login = { path = "../login" } -codex-linux-sandbox = { path = "../linux-sandbox" } codex-mcp-server = { path = "../mcp-server" } codex-tui = { path = "../tui" } serde_json = "1" diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 7916a7dc79..efda03bda4 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -2,6 +2,7 @@ use clap::CommandFactory; use clap::Parser; use clap_complete::Shell; use clap_complete::generate; +use codex_arg0::arg0_dispatch_or_else; use codex_chatgpt::apply_command::ApplyCommand; use codex_chatgpt::apply_command::run_apply_command; use codex_cli::LandlockCommand; @@ -92,7 +93,7 @@ struct LoginCommand { } fn main() -> anyhow::Result<()> { - codex_linux_sandbox::run_with_sandbox(|codex_linux_sandbox_exe| async move { + arg0_dispatch_or_else(|codex_linux_sandbox_exe| async move { cli_main(codex_linux_sandbox_exe).await?; Ok(()) }) diff --git a/codex-rs/exec/Cargo.toml b/codex-rs/exec/Cargo.toml index ed01b78ec8..c9d94deb5a 100644 --- a/codex-rs/exec/Cargo.toml +++ b/codex-rs/exec/Cargo.toml @@ -18,13 +18,13 @@ workspace = true anyhow = "1" chrono = "0.4.40" clap = { version = "4", features = ["derive"] } +codex-arg0 = { path = "../arg0" } codex-core = { path = "../core" } codex-common = { path = "../common", features = [ "cli", "elapsed", "sandbox_summary", ] } -codex-linux-sandbox = { path = "../linux-sandbox" } owo-colors = "4.2.0" serde_json = "1" shlex = "1.3.0" diff --git a/codex-rs/exec/src/main.rs b/codex-rs/exec/src/main.rs index 3a8e1f9411..03ee533ea9 100644 --- a/codex-rs/exec/src/main.rs +++ b/codex-rs/exec/src/main.rs @@ -10,6 +10,7 @@ //! This allows us to ship a completely separate set of functionality as part //! of the `codex-exec` binary. use clap::Parser; +use codex_arg0::arg0_dispatch_or_else; use codex_common::CliConfigOverrides; use codex_exec::Cli; use codex_exec::run_main; @@ -24,7 +25,7 @@ struct TopCli { } fn main() -> anyhow::Result<()> { - codex_linux_sandbox::run_with_sandbox(|codex_linux_sandbox_exe| async move { + arg0_dispatch_or_else(|codex_linux_sandbox_exe| async move { let top_cli = TopCli::parse(); // Merge root-level overrides into inner CLI struct so downstream logic remains unchanged. let mut inner = top_cli.inner; diff --git a/codex-rs/linux-sandbox/Cargo.toml b/codex-rs/linux-sandbox/Cargo.toml index 5c2dea6083..4b173ea17a 100644 --- a/codex-rs/linux-sandbox/Cargo.toml +++ b/codex-rs/linux-sandbox/Cargo.toml @@ -14,15 +14,16 @@ path = "src/lib.rs" [lints] workspace = true -[dependencies] +[target.'cfg(target_os = "linux")'.dependencies] anyhow = "1" clap = { version = "4", features = ["derive"] } codex-common = { path = "../common", features = ["cli"] } codex-core = { path = "../core" } -dotenvy = "0.15.7" -tokio = { version = "1", features = ["rt-multi-thread"] } +libc = "0.2.172" +landlock = "0.4.1" +seccompiler = "0.5.0" -[dev-dependencies] +[target.'cfg(target_os = "linux")'.dev-dependencies] tempfile = "3" tokio = { version = "1", features = [ "io-std", @@ -31,8 +32,3 @@ tokio = { version = "1", features = [ "rt-multi-thread", "signal", ] } - -[target.'cfg(target_os = "linux")'.dependencies] -libc = "0.2.172" -landlock = "0.4.1" -seccompiler = "0.5.0" diff --git a/codex-rs/linux-sandbox/src/lib.rs b/codex-rs/linux-sandbox/src/lib.rs index 960678467c..80453c7f96 100644 --- a/codex-rs/linux-sandbox/src/lib.rs +++ b/codex-rs/linux-sandbox/src/lib.rs @@ -4,72 +4,11 @@ mod landlock; mod linux_run_main; #[cfg(target_os = "linux")] -pub use linux_run_main::run_main; - -use std::future::Future; -use std::path::PathBuf; - -/// Helper that consolidates the common boilerplate found in several Codex -/// binaries (`codex`, `codex-exec`, `codex-tui`) around dispatching to the -/// `codex-linux-sandbox` sub-command. -/// -/// When the current executable is invoked through the hard-link or alias -/// named `codex-linux-sandbox` we *directly* execute [`run_main`](crate::run_main) -/// (which never returns). Otherwise we: -/// 1. Construct a Tokio multi-thread runtime. -/// 2. Derive the path to the current executable (so children can re-invoke -/// the sandbox) when running on Linux. -/// 3. Execute the provided async `main_fn` inside that runtime, forwarding -/// any error. -/// -/// This function eliminates duplicated code across the various `main.rs` -/// entry-points. -pub fn run_with_sandbox(main_fn: F) -> anyhow::Result<()> -where - F: FnOnce(Option) -> Fut, - Fut: Future>, -{ - use std::path::Path; - - // Determine if we were invoked via the special alias. - let argv0 = std::env::args().next().unwrap_or_default(); - let exe_name = Path::new(&argv0) - .file_name() - .and_then(|s| s.to_str()) - .unwrap_or(""); - - if exe_name == "codex-linux-sandbox" { - // Safety: [`run_main`] never returns. - crate::run_main(); - } - - // This modifies the environment, which is not thread-safe, so do this - // before creating any threads/the Tokio runtime. - load_dotenv(); - - // Regular invocation – create a Tokio runtime and execute the provided - // async entry-point. - let runtime = tokio::runtime::Runtime::new()?; - runtime.block_on(async move { - let codex_linux_sandbox_exe: Option = if cfg!(target_os = "linux") { - std::env::current_exe().ok() - } else { - None - }; - - main_fn(codex_linux_sandbox_exe).await - }) +pub fn run_main() -> ! { + linux_run_main::run_main(); } #[cfg(not(target_os = "linux"))] pub fn run_main() -> ! { panic!("codex-linux-sandbox is only supported on Linux"); } - -/// Load env vars from ~/.codex/.env and `$(pwd)/.env`. -fn load_dotenv() { - if let Ok(codex_home) = codex_core::config::find_codex_home() { - dotenvy::from_path(codex_home.join(".env")).ok(); - } - dotenvy::dotenv().ok(); -} diff --git a/codex-rs/mcp-server/Cargo.toml b/codex-rs/mcp-server/Cargo.toml index 1088b92481..488ee6a67c 100644 --- a/codex-rs/mcp-server/Cargo.toml +++ b/codex-rs/mcp-server/Cargo.toml @@ -16,8 +16,8 @@ workspace = true [dependencies] anyhow = "1" +codex-arg0 = { path = "../arg0" } codex-core = { path = "../core" } -codex-linux-sandbox = { path = "../linux-sandbox" } mcp-types = { path = "../mcp-types" } schemars = "0.8.22" serde = { version = "1", features = ["derive"] } diff --git a/codex-rs/mcp-server/src/main.rs b/codex-rs/mcp-server/src/main.rs index 51c46c44d2..60ddeeab41 100644 --- a/codex-rs/mcp-server/src/main.rs +++ b/codex-rs/mcp-server/src/main.rs @@ -1,7 +1,8 @@ +use codex_arg0::arg0_dispatch_or_else; use codex_mcp_server::run_main; fn main() -> anyhow::Result<()> { - codex_linux_sandbox::run_with_sandbox(|codex_linux_sandbox_exe| async move { + arg0_dispatch_or_else(|codex_linux_sandbox_exe| async move { run_main(codex_linux_sandbox_exe).await?; Ok(()) }) diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index b88ac8a080..2f150921fb 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -19,6 +19,7 @@ anyhow = "1" base64 = "0.22.1" clap = { version = "4", features = ["derive"] } codex-ansi-escape = { path = "../ansi-escape" } +codex-arg0 = { path = "../arg0" } codex-core = { path = "../core" } codex-common = { path = "../common", features = [ "cli", @@ -26,7 +27,6 @@ codex-common = { path = "../common", features = [ "sandbox_summary", ] } codex-file-search = { path = "../file-search" } -codex-linux-sandbox = { path = "../linux-sandbox" } codex-login = { path = "../login" } color-eyre = "0.6.3" crossterm = { version = "0.28.1", features = ["bracketed-paste"] } diff --git a/codex-rs/tui/src/main.rs b/codex-rs/tui/src/main.rs index fdb3cdaf82..480e56e88e 100644 --- a/codex-rs/tui/src/main.rs +++ b/codex-rs/tui/src/main.rs @@ -1,4 +1,5 @@ use clap::Parser; +use codex_arg0::arg0_dispatch_or_else; use codex_common::CliConfigOverrides; use codex_tui::Cli; use codex_tui::run_main; @@ -13,7 +14,7 @@ struct TopCli { } fn main() -> anyhow::Result<()> { - codex_linux_sandbox::run_with_sandbox(|codex_linux_sandbox_exe| async move { + arg0_dispatch_or_else(|codex_linux_sandbox_exe| async move { let top_cli = TopCli::parse(); let mut inner = top_cli.inner; inner From fc25aa6e051e2518ad7fd85f9f0c98bb0d665187 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Jul 2025 08:32:57 -0700 Subject: [PATCH 3/6] fix: use std::env::args_os instead of std::env::args --- codex-rs/arg0/src/lib.rs | 2 +- codex-rs/core/src/mcp_connection_manager.rs | 8 +++++++- codex-rs/mcp-client/src/main.rs | 3 ++- codex-rs/mcp-client/src/mcp_client.rs | 5 +++-- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index 86b98c7d30..624583b8aa 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -30,7 +30,7 @@ where Fut: Future>, { // Determine if we were invoked via the special alias. - let argv0 = std::env::args().next().unwrap_or_default(); + let argv0 = std::env::args_os().next().unwrap_or_default(); let exe_name = Path::new(&argv0) .file_name() .and_then(|s| s.to_str()) diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 886e4f8bf7..2e33c8754b 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -8,6 +8,7 @@ use std::collections::HashMap; use std::collections::HashSet; +use std::ffi::OsString; use std::time::Duration; use anyhow::Context; @@ -127,7 +128,12 @@ impl McpConnectionManager { join_set.spawn(async move { let McpServerConfig { command, args, env } = cfg; - let client_res = McpClient::new_stdio_client(command, args, env).await; + let client_res = McpClient::new_stdio_client( + command.into(), + args.into_iter().map(OsString::from).collect(), + env, + ) + .await; match client_res { Ok(client) => { // Initialize the client. diff --git a/codex-rs/mcp-client/src/main.rs b/codex-rs/mcp-client/src/main.rs index 8d671b830f..10cfe389bf 100644 --- a/codex-rs/mcp-client/src/main.rs +++ b/codex-rs/mcp-client/src/main.rs @@ -10,6 +10,7 @@ //! program. The utility connects, issues a `tools/list` request and prints the //! server's response as pretty JSON. +use std::ffi::OsString; use std::time::Duration; use anyhow::Context; @@ -37,7 +38,7 @@ async fn main() -> Result<()> { .try_init(); // Collect command-line arguments excluding the program name itself. - let mut args: Vec = std::env::args().skip(1).collect(); + let mut args: Vec = std::env::args_os().skip(1).collect(); if args.is_empty() || args[0] == "--help" || args[0] == "-h" { eprintln!("Usage: mcp-client [args..]\n\nExample: mcp-client codex-mcp-server"); diff --git a/codex-rs/mcp-client/src/mcp_client.rs b/codex-rs/mcp-client/src/mcp_client.rs index 6a9111e69f..084d0bf4ba 100644 --- a/codex-rs/mcp-client/src/mcp_client.rs +++ b/codex-rs/mcp-client/src/mcp_client.rs @@ -12,6 +12,7 @@ //! issue requests and receive strongly-typed results. use std::collections::HashMap; +use std::ffi::OsString; use std::sync::Arc; use std::sync::atomic::AtomicI64; use std::sync::atomic::Ordering; @@ -82,8 +83,8 @@ impl McpClient { /// Caller is responsible for sending the `initialize` request. See /// [`initialize`](Self::initialize) for details. pub async fn new_stdio_client( - program: String, - args: Vec, + program: OsString, + args: Vec, env: Option>, ) -> std::io::Result { let mut child = Command::new(program) From bdb7f8e138505646cdd71973d2daa605d73f8288 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Jul 2025 08:32:57 -0700 Subject: [PATCH 4/6] fix: support special --codex-run-as-apply-patch arg --- codex-rs/Cargo.lock | 4 ++++ codex-rs/arg0/Cargo.toml | 1 + codex-rs/arg0/src/lib.rs | 23 +++++++++++++++++- codex-rs/core/README.md | 17 +++++++++---- codex-rs/exec/Cargo.toml | 5 ++++ codex-rs/exec/tests/apply_patch.rs | 38 ++++++++++++++++++++++++++++++ 6 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 codex-rs/exec/tests/apply_patch.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index f3903d6ea6..653c3e4ef2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -610,6 +610,7 @@ name = "codex-arg0" version = "0.0.0" dependencies = [ "anyhow", + "codex-apply-patch", "codex-core", "codex-linux-sandbox", "dotenvy", @@ -718,14 +719,17 @@ name = "codex-exec" version = "0.0.0" dependencies = [ "anyhow", + "assert_cmd", "chrono", "clap", "codex-arg0", "codex-common", "codex-core", "owo-colors", + "predicates", "serde_json", "shlex", + "tempfile", "tokio", "tracing", "tracing-subscriber", diff --git a/codex-rs/arg0/Cargo.toml b/codex-rs/arg0/Cargo.toml index 9ad1896746..7c55ac0d96 100644 --- a/codex-rs/arg0/Cargo.toml +++ b/codex-rs/arg0/Cargo.toml @@ -12,6 +12,7 @@ workspace = true [dependencies] anyhow = "1" +codex-apply-patch = { path = "../apply-patch" } codex-core = { path = "../core" } codex-linux-sandbox = { path = "../linux-sandbox" } dotenvy = "0.15.7" diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index 624583b8aa..d7109176a5 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -30,7 +30,8 @@ where Fut: Future>, { // Determine if we were invoked via the special alias. - let argv0 = std::env::args_os().next().unwrap_or_default(); + let mut args = std::env::args_os(); + let argv0 = args.next().unwrap_or_default(); let exe_name = Path::new(&argv0) .file_name() .and_then(|s| s.to_str()) @@ -41,6 +42,26 @@ where codex_linux_sandbox::run_main(); } + let argv1 = args.next().unwrap_or_default(); + if argv1 == "--codex-run-as-apply-patch" { + let patch_arg = args.next().and_then(|s| s.to_str().map(|s| s.to_owned())); + let exit_code = match patch_arg { + Some(patch_arg) => { + let mut stdout = std::io::stdout(); + let mut stderr = std::io::stderr(); + match codex_apply_patch::apply_patch(&patch_arg, &mut stdout, &mut stderr) { + Ok(()) => 0, + Err(_) => 1, + } + } + None => { + eprintln!("Error: --codex-run-as-apply-patch requires a UTF-8 PATCH argument."); + 1 + } + }; + std::process::exit(exit_code); + } + // This modifies the environment, which is not thread-safe, so do this // before creating any threads/the Tokio runtime. load_dotenv(); diff --git a/codex-rs/core/README.md b/codex-rs/core/README.md index 9b3e59c8af..9a4c255abe 100644 --- a/codex-rs/core/README.md +++ b/codex-rs/core/README.md @@ -2,9 +2,18 @@ This crate implements the business logic for Codex. It is designed to be used by the various Codex UIs written in Rust. -Though for non-Rust UIs, we are also working to define a _protocol_ for talking to Codex. See: +## Dependencies -- [Specification](../docs/protocol_v1.md) -- [Rust types](./src/protocol.rs) +Note that `codex-core` makes some assumptions about certain helper utilities being available in the environment. Currently, this -You can use the `proto` subcommand using the executable in the [`cli` crate](../cli) to speak the protocol using newline-delimited-JSON over stdin/stdout. +### macOS + +Expects `/usr/bin/sandbox-exec` to be present. + +### Linux + +Expects the binary containing `codex-core` to run the equivalent of `codex debug landlock` when `arg0` is `codex-linux-sandbox`. See the `codex-arg0` crate for details. + +### All Platforms + +Expects the binary containing `codex-core` to simulate the virtual `apply_patch` CLI when `arg1` is `--codex-run-as-apply-patch`. See the `codex-arg0` crate for details. diff --git a/codex-rs/exec/Cargo.toml b/codex-rs/exec/Cargo.toml index c9d94deb5a..ced771f238 100644 --- a/codex-rs/exec/Cargo.toml +++ b/codex-rs/exec/Cargo.toml @@ -37,3 +37,8 @@ tokio = { version = "1", features = [ ] } tracing = { version = "0.1.41", features = ["log"] } tracing-subscriber = { version = "0.3.19", features = ["env-filter"] } + +[dev-dependencies] +assert_cmd = "2" +predicates = "3" +tempfile = "3.13.0" diff --git a/codex-rs/exec/tests/apply_patch.rs b/codex-rs/exec/tests/apply_patch.rs new file mode 100644 index 0000000000..69ac1b8c0a --- /dev/null +++ b/codex-rs/exec/tests/apply_patch.rs @@ -0,0 +1,38 @@ +use anyhow::Context; +use assert_cmd::prelude::*; +use std::fs; +use std::process::Command; +use tempfile::tempdir; + +/// While we may add an `apply-patch` subcommand to the `codex` CLI multitool +/// at some point, we must ensure that the smaller `codex-exec` CLI can still +/// emulate the `apply_patch` CLI. +#[test] +fn test_standalone_exec_cli_can_use_apply_patch() -> anyhow::Result<()> { + let tmp = tempdir()?; + let relative_path = "source.txt"; + let absolute_path = tmp.path().join(relative_path); + fs::write(&absolute_path, "original content\n")?; + + Command::cargo_bin("codex-exec") + .context("should find binary for codex-exec")? + .arg("--codex-run-as-apply-patch") + .arg( + r#"*** Begin Patch +*** Update File: source.txt +@@ +-original content ++modified by apply_patch +*** End Patch"#, + ) + .current_dir(tmp.path()) + .assert() + .success() + .stdout("Success. Updated the following files:\nM source.txt\n") + .stderr(predicates::str::is_empty()); + assert_eq!( + fs::read_to_string(absolute_path)?, + "modified by apply_patch\n" + ); + Ok(()) +} From 2d190ae8c5f0b4f2be3956ba6b6d19293d849b70 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Jul 2025 08:32:57 -0700 Subject: [PATCH 5/6] chore: split apply_patch logic out of codex.rs and into apply_patch.rs --- codex-rs/core/src/apply_patch.rs | 406 +++++++++++++++++++++++++++++++ codex-rs/core/src/codex.rs | 400 +----------------------------- codex-rs/core/src/lib.rs | 1 + 3 files changed, 415 insertions(+), 392 deletions(-) create mode 100644 codex-rs/core/src/apply_patch.rs diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs new file mode 100644 index 0000000000..44af72c746 --- /dev/null +++ b/codex-rs/core/src/apply_patch.rs @@ -0,0 +1,406 @@ +use crate::codex::Session; +use crate::models::FunctionCallOutputPayload; +use crate::models::ResponseInputItem; +use crate::protocol::Event; +use crate::protocol::EventMsg; +use crate::protocol::FileChange; +use crate::protocol::PatchApplyBeginEvent; +use crate::protocol::PatchApplyEndEvent; +use crate::protocol::ReviewDecision; +use crate::safety::SafetyCheck; +use crate::safety::assess_patch_safety; +use anyhow::Context; +use codex_apply_patch::AffectedPaths; +use codex_apply_patch::ApplyPatchAction; +use codex_apply_patch::ApplyPatchFileChange; +use codex_apply_patch::print_summary; +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; + +pub(crate) async fn apply_patch( + sess: &Session, + sub_id: String, + call_id: String, + action: ApplyPatchAction, +) -> ResponseInputItem { + let writable_roots_snapshot = { + #[allow(clippy::unwrap_used)] + let guard = sess.writable_roots.lock().unwrap(); + guard.clone() + }; + + let auto_approved = match assess_patch_safety( + &action, + sess.approval_policy, + &writable_roots_snapshot, + &sess.cwd, + ) { + SafetyCheck::AutoApprove { .. } => true, + SafetyCheck::AskUser => { + // Compute a readable summary of path changes to include in the + // approval request so the user can make an informed decision. + let rx_approve = sess + .request_patch_approval(sub_id.clone(), call_id.clone(), &action, None, None) + .await; + match rx_approve.await.unwrap_or_default() { + ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, + ReviewDecision::Denied | ReviewDecision::Abort => { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: "patch rejected by user".to_string(), + success: Some(false), + }, + }; + } + } + } + SafetyCheck::Reject { reason } => { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!("patch rejected: {reason}"), + success: Some(false), + }, + }; + } + }; + + // Verify write permissions before touching the filesystem. + let writable_snapshot = { + #[allow(clippy::unwrap_used)] + sess.writable_roots.lock().unwrap().clone() + }; + + if let Some(offending) = first_offending_path(&action, &writable_snapshot, &sess.cwd) { + let root = offending.parent().unwrap_or(&offending).to_path_buf(); + + let reason = Some(format!( + "grant write access to {} for this session", + root.display() + )); + + let rx = sess + .request_patch_approval( + sub_id.clone(), + call_id.clone(), + &action, + reason.clone(), + Some(root.clone()), + ) + .await; + + if !matches!( + rx.await.unwrap_or_default(), + ReviewDecision::Approved | ReviewDecision::ApprovedForSession + ) { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: "patch rejected by user".to_string(), + success: Some(false), + }, + }; + } + + // user approved, extend writable roots for this session + #[allow(clippy::unwrap_used)] + sess.writable_roots.lock().unwrap().push(root); + } + + let _ = sess + .tx_event + .send(Event { + id: sub_id.clone(), + msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { + call_id: call_id.clone(), + auto_approved, + changes: convert_apply_patch_to_protocol(&action), + }), + }) + .await; + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + // Enforce writable roots. If a write is blocked, collect offending root + // and prompt the user to extend permissions. + let mut result = apply_changes_from_apply_patch_and_report(&action, &mut stdout, &mut stderr); + + if let Err(err) = &result { + if err.kind() == std::io::ErrorKind::PermissionDenied { + // Determine first offending path. + let offending_opt = action + .changes() + .iter() + .flat_map(|(path, change)| match change { + ApplyPatchFileChange::Add { .. } => vec![path.as_ref()], + ApplyPatchFileChange::Delete => vec![path.as_ref()], + ApplyPatchFileChange::Update { + move_path: Some(move_path), + .. + } => { + vec![path.as_ref(), move_path.as_ref()] + } + ApplyPatchFileChange::Update { + move_path: None, .. + } => vec![path.as_ref()], + }) + .find_map(|path: &Path| { + // ApplyPatchAction promises to guarantee absolute paths. + if !path.is_absolute() { + panic!("apply_patch invariant failed: path is not absolute: {path:?}"); + } + + let writable = { + #[allow(clippy::unwrap_used)] + let roots = sess.writable_roots.lock().unwrap(); + roots.iter().any(|root| path.starts_with(root)) + }; + if writable { + None + } else { + Some(path.to_path_buf()) + } + }); + + if let Some(offending) = offending_opt { + let root = offending.parent().unwrap_or(&offending).to_path_buf(); + + let reason = Some(format!( + "grant write access to {} for this session", + root.display() + )); + let rx = sess + .request_patch_approval( + sub_id.clone(), + call_id.clone(), + &action, + reason.clone(), + Some(root.clone()), + ) + .await; + if matches!( + rx.await.unwrap_or_default(), + ReviewDecision::Approved | ReviewDecision::ApprovedForSession + ) { + // Extend writable roots. + #[allow(clippy::unwrap_used)] + sess.writable_roots.lock().unwrap().push(root); + stdout.clear(); + stderr.clear(); + result = apply_changes_from_apply_patch_and_report( + &action, + &mut stdout, + &mut stderr, + ); + } + } + } + } + + // Emit PatchApplyEnd event. + let success_flag = result.is_ok(); + let _ = sess + .tx_event + .send(Event { + id: sub_id.clone(), + msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { + call_id: call_id.clone(), + stdout: String::from_utf8_lossy(&stdout).to_string(), + stderr: String::from_utf8_lossy(&stderr).to_string(), + success: success_flag, + }), + }) + .await; + + match result { + Ok(_) => ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: String::from_utf8_lossy(&stdout).to_string(), + success: None, + }, + }, + Err(e) => ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!("error: {e:#}, stderr: {}", String::from_utf8_lossy(&stderr)), + success: Some(false), + }, + }, + } +} + +/// Return the first path in `hunks` that is NOT under any of the +/// `writable_roots` (after normalising). If all paths are acceptable, +/// returns None. +fn first_offending_path( + action: &ApplyPatchAction, + writable_roots: &[PathBuf], + cwd: &Path, +) -> Option { + let changes = action.changes(); + for (path, change) in changes { + let candidate = match change { + ApplyPatchFileChange::Add { .. } => path, + ApplyPatchFileChange::Delete => path, + ApplyPatchFileChange::Update { move_path, .. } => move_path.as_ref().unwrap_or(path), + }; + + let abs = if candidate.is_absolute() { + candidate.clone() + } else { + cwd.join(candidate) + }; + + let mut allowed = false; + for root in writable_roots { + let root_abs = if root.is_absolute() { + root.clone() + } else { + cwd.join(root) + }; + if abs.starts_with(&root_abs) { + allowed = true; + break; + } + } + + if !allowed { + return Some(candidate.clone()); + } + } + None +} + +pub(crate) fn convert_apply_patch_to_protocol( + action: &ApplyPatchAction, +) -> HashMap { + let changes = action.changes(); + let mut result = HashMap::with_capacity(changes.len()); + for (path, change) in changes { + let protocol_change = match change { + ApplyPatchFileChange::Add { content } => FileChange::Add { + content: content.clone(), + }, + ApplyPatchFileChange::Delete => FileChange::Delete, + ApplyPatchFileChange::Update { + unified_diff, + move_path, + new_content: _new_content, + } => FileChange::Update { + unified_diff: unified_diff.clone(), + move_path: move_path.clone(), + }, + }; + result.insert(path.clone(), protocol_change); + } + result +} + +fn apply_changes_from_apply_patch_and_report( + action: &ApplyPatchAction, + stdout: &mut impl std::io::Write, + stderr: &mut impl std::io::Write, +) -> std::io::Result<()> { + match apply_changes_from_apply_patch(action) { + Ok(affected_paths) => { + print_summary(&affected_paths, stdout)?; + } + Err(err) => { + writeln!(stderr, "{err:?}")?; + } + } + + Ok(()) +} + +fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result { + let mut added: Vec = Vec::new(); + let mut modified: Vec = Vec::new(); + let mut deleted: Vec = Vec::new(); + + let changes = action.changes(); + for (path, change) in changes { + match change { + ApplyPatchFileChange::Add { content } => { + if let Some(parent) = path.parent() { + if !parent.as_os_str().is_empty() { + std::fs::create_dir_all(parent).with_context(|| { + format!("Failed to create parent directories for {}", path.display()) + })?; + } + } + std::fs::write(path, content) + .with_context(|| format!("Failed to write file {}", path.display()))?; + added.push(path.clone()); + } + ApplyPatchFileChange::Delete => { + std::fs::remove_file(path) + .with_context(|| format!("Failed to delete file {}", path.display()))?; + deleted.push(path.clone()); + } + ApplyPatchFileChange::Update { + unified_diff: _unified_diff, + move_path, + new_content, + } => { + if let Some(move_path) = move_path { + if let Some(parent) = move_path.parent() { + if !parent.as_os_str().is_empty() { + std::fs::create_dir_all(parent).with_context(|| { + format!( + "Failed to create parent directories for {}", + move_path.display() + ) + })?; + } + } + + std::fs::rename(path, move_path) + .with_context(|| format!("Failed to rename file {}", path.display()))?; + std::fs::write(move_path, new_content)?; + modified.push(move_path.clone()); + deleted.push(path.clone()); + } else { + std::fs::write(path, new_content)?; + modified.push(path.clone()); + } + } + } + } + + Ok(AffectedPaths { + added, + modified, + deleted, + }) +} + +pub(crate) fn get_writable_roots(cwd: &Path) -> Vec { + let mut writable_roots = Vec::new(); + if cfg!(target_os = "macos") { + // On macOS, $TMPDIR is private to the user. + writable_roots.push(std::env::temp_dir()); + + // Allow pyenv to update its shims directory. Without this, any tool + // that happens to be managed by `pyenv` will fail with an error like: + // + // pyenv: cannot rehash: $HOME/.pyenv/shims isn't writable + // + // which is emitted every time `pyenv` tries to run `rehash` (for + // example, after installing a new Python package that drops an entry + // point). Although the sandbox is intentionally read‑only by default, + // writing to the user's local `pyenv` directory is safe because it + // is already user‑writable and scoped to the current user account. + if let Ok(home_dir) = std::env::var("HOME") { + let pyenv_dir = PathBuf::from(home_dir).join(".pyenv"); + writable_roots.push(pyenv_dir); + } + } + + writable_roots.push(cwd.to_path_buf()); + + writable_roots +} diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 5764440e79..3ab3e8d780 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4,22 +4,17 @@ use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; -use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; use std::sync::atomic::AtomicU64; use std::time::Duration; -use anyhow::Context; use async_channel::Receiver; use async_channel::Sender; -use codex_apply_patch::AffectedPaths; use codex_apply_patch::ApplyPatchAction; -use codex_apply_patch::ApplyPatchFileChange; use codex_apply_patch::MaybeApplyPatchVerified; use codex_apply_patch::maybe_parse_apply_patch_verified; -use codex_apply_patch::print_summary; use futures::prelude::*; use mcp_types::CallToolResult; use serde::Serialize; @@ -34,6 +29,9 @@ use tracing::trace; use tracing::warn; use uuid::Uuid; +use crate::apply_patch::convert_apply_patch_to_protocol; +use crate::apply_patch::get_writable_roots; +use crate::apply_patch::{self}; use crate::client::ModelClient; use crate::client_common::Prompt; use crate::client_common::ResponseEvent; @@ -71,11 +69,8 @@ use crate::protocol::EventMsg; use crate::protocol::ExecApprovalRequestEvent; use crate::protocol::ExecCommandBeginEvent; use crate::protocol::ExecCommandEndEvent; -use crate::protocol::FileChange; use crate::protocol::InputItem; use crate::protocol::Op; -use crate::protocol::PatchApplyBeginEvent; -use crate::protocol::PatchApplyEndEvent; use crate::protocol::ReviewDecision; use crate::protocol::SandboxPolicy; use crate::protocol::SessionConfiguredEvent; @@ -84,7 +79,6 @@ use crate::protocol::TaskCompleteEvent; use crate::rollout::RolloutRecorder; use crate::safety::SafetyCheck; use crate::safety::assess_command_safety; -use crate::safety::assess_patch_safety; use crate::shell; use crate::user_notification::UserNotification; use crate::util::backoff; @@ -189,19 +183,19 @@ impl Codex { /// A session has at most 1 running task at a time, and can be interrupted by user input. pub(crate) struct Session { client: ModelClient, - tx_event: Sender, + pub(crate) tx_event: Sender, ctrl_c: Arc, /// The session's current working directory. All relative paths provided by /// the model as well as sandbox policies are resolved against this path /// instead of `std::env::current_dir()`. - cwd: PathBuf, + pub(crate) cwd: PathBuf, base_instructions: Option, user_instructions: Option, - approval_policy: AskForApproval, + pub(crate) approval_policy: AskForApproval, sandbox_policy: SandboxPolicy, shell_environment_policy: ShellEnvironmentPolicy, - writable_roots: Mutex>, + pub(crate) writable_roots: Mutex>, disable_response_storage: bool, /// Manager for external MCP servers/tools. @@ -1419,7 +1413,7 @@ async fn handle_container_exec_with_params( // check if this was a patch, and apply it if so match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { MaybeApplyPatchVerified::Body(changes) => { - return apply_patch(sess, sub_id, call_id, changes).await; + return apply_patch::apply_patch(sess, sub_id, call_id, changes).await; } MaybeApplyPatchVerified::CorrectnessError(parse_error) => { // It looks like an invocation of `apply_patch`, but we @@ -1668,384 +1662,6 @@ async fn handle_sandbox_error( } } -async fn apply_patch( - sess: &Session, - sub_id: String, - call_id: String, - action: ApplyPatchAction, -) -> ResponseInputItem { - let writable_roots_snapshot = { - let guard = sess.writable_roots.lock().unwrap(); - guard.clone() - }; - - let auto_approved = match assess_patch_safety( - &action, - sess.approval_policy, - &writable_roots_snapshot, - &sess.cwd, - ) { - SafetyCheck::AutoApprove { .. } => true, - SafetyCheck::AskUser => { - // Compute a readable summary of path changes to include in the - // approval request so the user can make an informed decision. - let rx_approve = sess - .request_patch_approval(sub_id.clone(), call_id.clone(), &action, None, None) - .await; - match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, - ReviewDecision::Denied | ReviewDecision::Abort => { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: "patch rejected by user".to_string(), - success: Some(false), - }, - }; - } - } - } - SafetyCheck::Reject { reason } => { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("patch rejected: {reason}"), - success: Some(false), - }, - }; - } - }; - - // Verify write permissions before touching the filesystem. - let writable_snapshot = { sess.writable_roots.lock().unwrap().clone() }; - - if let Some(offending) = first_offending_path(&action, &writable_snapshot, &sess.cwd) { - let root = offending.parent().unwrap_or(&offending).to_path_buf(); - - let reason = Some(format!( - "grant write access to {} for this session", - root.display() - )); - - let rx = sess - .request_patch_approval( - sub_id.clone(), - call_id.clone(), - &action, - reason.clone(), - Some(root.clone()), - ) - .await; - - if !matches!( - rx.await.unwrap_or_default(), - ReviewDecision::Approved | ReviewDecision::ApprovedForSession - ) { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: "patch rejected by user".to_string(), - success: Some(false), - }, - }; - } - - // user approved, extend writable roots for this session - sess.writable_roots.lock().unwrap().push(root); - } - - let _ = sess - .tx_event - .send(Event { - id: sub_id.clone(), - msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { - call_id: call_id.clone(), - auto_approved, - changes: convert_apply_patch_to_protocol(&action), - }), - }) - .await; - - let mut stdout = Vec::new(); - let mut stderr = Vec::new(); - // Enforce writable roots. If a write is blocked, collect offending root - // and prompt the user to extend permissions. - let mut result = apply_changes_from_apply_patch_and_report(&action, &mut stdout, &mut stderr); - - if let Err(err) = &result { - if err.kind() == std::io::ErrorKind::PermissionDenied { - // Determine first offending path. - let offending_opt = action - .changes() - .iter() - .flat_map(|(path, change)| match change { - ApplyPatchFileChange::Add { .. } => vec![path.as_ref()], - ApplyPatchFileChange::Delete => vec![path.as_ref()], - ApplyPatchFileChange::Update { - move_path: Some(move_path), - .. - } => { - vec![path.as_ref(), move_path.as_ref()] - } - ApplyPatchFileChange::Update { - move_path: None, .. - } => vec![path.as_ref()], - }) - .find_map(|path: &Path| { - // ApplyPatchAction promises to guarantee absolute paths. - if !path.is_absolute() { - panic!("apply_patch invariant failed: path is not absolute: {path:?}"); - } - - let writable = { - let roots = sess.writable_roots.lock().unwrap(); - roots.iter().any(|root| path.starts_with(root)) - }; - if writable { - None - } else { - Some(path.to_path_buf()) - } - }); - - if let Some(offending) = offending_opt { - let root = offending.parent().unwrap_or(&offending).to_path_buf(); - - let reason = Some(format!( - "grant write access to {} for this session", - root.display() - )); - let rx = sess - .request_patch_approval( - sub_id.clone(), - call_id.clone(), - &action, - reason.clone(), - Some(root.clone()), - ) - .await; - if matches!( - rx.await.unwrap_or_default(), - ReviewDecision::Approved | ReviewDecision::ApprovedForSession - ) { - // Extend writable roots. - sess.writable_roots.lock().unwrap().push(root); - stdout.clear(); - stderr.clear(); - result = apply_changes_from_apply_patch_and_report( - &action, - &mut stdout, - &mut stderr, - ); - } - } - } - } - - // Emit PatchApplyEnd event. - let success_flag = result.is_ok(); - let _ = sess - .tx_event - .send(Event { - id: sub_id.clone(), - msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { - call_id: call_id.clone(), - stdout: String::from_utf8_lossy(&stdout).to_string(), - stderr: String::from_utf8_lossy(&stderr).to_string(), - success: success_flag, - }), - }) - .await; - - match result { - Ok(_) => ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: String::from_utf8_lossy(&stdout).to_string(), - success: None, - }, - }, - Err(e) => ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("error: {e:#}, stderr: {}", String::from_utf8_lossy(&stderr)), - success: Some(false), - }, - }, - } -} - -/// Return the first path in `hunks` that is NOT under any of the -/// `writable_roots` (after normalising). If all paths are acceptable, -/// returns None. -fn first_offending_path( - action: &ApplyPatchAction, - writable_roots: &[PathBuf], - cwd: &Path, -) -> Option { - let changes = action.changes(); - for (path, change) in changes { - let candidate = match change { - ApplyPatchFileChange::Add { .. } => path, - ApplyPatchFileChange::Delete => path, - ApplyPatchFileChange::Update { move_path, .. } => move_path.as_ref().unwrap_or(path), - }; - - let abs = if candidate.is_absolute() { - candidate.clone() - } else { - cwd.join(candidate) - }; - - let mut allowed = false; - for root in writable_roots { - let root_abs = if root.is_absolute() { - root.clone() - } else { - cwd.join(root) - }; - if abs.starts_with(&root_abs) { - allowed = true; - break; - } - } - - if !allowed { - return Some(candidate.clone()); - } - } - None -} - -fn convert_apply_patch_to_protocol(action: &ApplyPatchAction) -> HashMap { - let changes = action.changes(); - let mut result = HashMap::with_capacity(changes.len()); - for (path, change) in changes { - let protocol_change = match change { - ApplyPatchFileChange::Add { content } => FileChange::Add { - content: content.clone(), - }, - ApplyPatchFileChange::Delete => FileChange::Delete, - ApplyPatchFileChange::Update { - unified_diff, - move_path, - new_content: _new_content, - } => FileChange::Update { - unified_diff: unified_diff.clone(), - move_path: move_path.clone(), - }, - }; - result.insert(path.clone(), protocol_change); - } - result -} - -fn apply_changes_from_apply_patch_and_report( - action: &ApplyPatchAction, - stdout: &mut impl std::io::Write, - stderr: &mut impl std::io::Write, -) -> std::io::Result<()> { - match apply_changes_from_apply_patch(action) { - Ok(affected_paths) => { - print_summary(&affected_paths, stdout)?; - } - Err(err) => { - writeln!(stderr, "{err:?}")?; - } - } - - Ok(()) -} - -fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result { - let mut added: Vec = Vec::new(); - let mut modified: Vec = Vec::new(); - let mut deleted: Vec = Vec::new(); - - let changes = action.changes(); - for (path, change) in changes { - match change { - ApplyPatchFileChange::Add { content } => { - if let Some(parent) = path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent).with_context(|| { - format!("Failed to create parent directories for {}", path.display()) - })?; - } - } - std::fs::write(path, content) - .with_context(|| format!("Failed to write file {}", path.display()))?; - added.push(path.clone()); - } - ApplyPatchFileChange::Delete => { - std::fs::remove_file(path) - .with_context(|| format!("Failed to delete file {}", path.display()))?; - deleted.push(path.clone()); - } - ApplyPatchFileChange::Update { - unified_diff: _unified_diff, - move_path, - new_content, - } => { - if let Some(move_path) = move_path { - if let Some(parent) = move_path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent).with_context(|| { - format!( - "Failed to create parent directories for {}", - move_path.display() - ) - })?; - } - } - - std::fs::rename(path, move_path) - .with_context(|| format!("Failed to rename file {}", path.display()))?; - std::fs::write(move_path, new_content)?; - modified.push(move_path.clone()); - deleted.push(path.clone()); - } else { - std::fs::write(path, new_content)?; - modified.push(path.clone()); - } - } - } - } - - Ok(AffectedPaths { - added, - modified, - deleted, - }) -} - -fn get_writable_roots(cwd: &Path) -> Vec { - let mut writable_roots = Vec::new(); - if cfg!(target_os = "macos") { - // On macOS, $TMPDIR is private to the user. - writable_roots.push(std::env::temp_dir()); - - // Allow pyenv to update its shims directory. Without this, any tool - // that happens to be managed by `pyenv` will fail with an error like: - // - // pyenv: cannot rehash: $HOME/.pyenv/shims isn't writable - // - // which is emitted every time `pyenv` tries to run `rehash` (for - // example, after installing a new Python package that drops an entry - // point). Although the sandbox is intentionally read‑only by default, - // writing to the user's local `pyenv` directory is safe because it - // is already user‑writable and scoped to the current user account. - if let Ok(home_dir) = std::env::var("HOME") { - let pyenv_dir = PathBuf::from(home_dir).join(".pyenv"); - writable_roots.push(pyenv_dir); - } - } - - writable_roots.push(cwd.to_path_buf()); - - writable_roots -} - /// Exec output is a pre-serialized JSON payload fn format_exec_output(output: &str, exit_code: i32, duration: Duration) -> String { #[derive(Serialize)] diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index f390038c0c..6cb6aaa629 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -5,6 +5,7 @@ // the TUI or the tracing stack). #![deny(clippy::print_stdout, clippy::print_stderr)] +mod apply_patch; mod bash; mod chat_completions; mod client; From 99608d5e273ef20e8ec8d1bbb6256104d0ae93fe Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Jul 2025 08:32:57 -0700 Subject: [PATCH 6/6] fix: run apply_patch calls through the sandbox --- codex-rs/apply-patch/src/lib.rs | 62 ++++++++--- codex-rs/apply-patch/src/parser.rs | 54 +++++++--- codex-rs/core/src/apply_patch.rs | 63 +++++++---- codex-rs/core/src/codex.rs | 165 ++++++++++++++++++----------- 4 files changed, 226 insertions(+), 118 deletions(-) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index c81241d0da..1207f8830f 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -58,16 +58,22 @@ impl PartialEq for IoError { #[derive(Debug, PartialEq)] pub enum MaybeApplyPatch { - Body(Vec), + Body(ApplyPatchSource), ShellParseError(ExtractHeredocError), PatchParseError(ParseError), NotApplyPatch, } +#[derive(Debug, PartialEq)] +pub struct ApplyPatchSource { + pub hunks: Vec, + pub patch: String, +} + pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { match argv { [cmd, body] if cmd == "apply_patch" => match parse_patch(body) { - Ok(hunks) => MaybeApplyPatch::Body(hunks), + Ok(source) => MaybeApplyPatch::Body(source), Err(e) => MaybeApplyPatch::PatchParseError(e), }, [bash, flag, script] @@ -77,7 +83,7 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { { match extract_heredoc_body_from_apply_patch_command(script) { Ok(body) => match parse_patch(&body) { - Ok(hunks) => MaybeApplyPatch::Body(hunks), + Ok(source) => MaybeApplyPatch::Body(source), Err(e) => MaybeApplyPatch::PatchParseError(e), }, Err(e) => MaybeApplyPatch::ShellParseError(e), @@ -121,6 +127,14 @@ pub enum MaybeApplyPatchVerified { /// construction, all paths should be absolute paths. pub struct ApplyPatchAction { changes: HashMap, + + /// The raw patch argument that can be used with `apply_patch` as an exec + /// call. i.e., if the original arg was parsed in "lenient" mode with a + /// heredoc, this should be the value without the heredoc wrapper. + pub patch: String, + + /// The working directory that was used to resolve relative paths in the patch. + pub cwd: PathBuf, } impl ApplyPatchAction { @@ -140,8 +154,22 @@ impl ApplyPatchAction { panic!("path must be absolute"); } + #[allow(clippy::unwrap_used)] + let filename = path.file_name().unwrap().to_string_lossy(); + let patch = format!( + r#"*** Begin Patch +*** Update File: {filename} +@@ ++ {content} +*** End Patch"#, + ); let changes = HashMap::from([(path.to_path_buf(), ApplyPatchFileChange::Add { content })]); - Self { changes } + #[allow(clippy::unwrap_used)] + Self { + changes, + cwd: path.parent().unwrap().to_path_buf(), + patch, + } } } @@ -149,7 +177,7 @@ impl ApplyPatchAction { /// patch. pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApplyPatchVerified { match maybe_parse_apply_patch(argv) { - MaybeApplyPatch::Body(hunks) => { + MaybeApplyPatch::Body(ApplyPatchSource { patch, hunks }) => { let mut changes = HashMap::new(); for hunk in hunks { let path = hunk.resolve_path(cwd); @@ -183,7 +211,11 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp } } } - MaybeApplyPatchVerified::Body(ApplyPatchAction { changes }) + MaybeApplyPatchVerified::Body(ApplyPatchAction { + changes, + patch, + cwd: cwd.to_path_buf(), + }) } MaybeApplyPatch::ShellParseError(e) => MaybeApplyPatchVerified::ShellParseError(e), MaybeApplyPatch::PatchParseError(e) => MaybeApplyPatchVerified::CorrectnessError(e.into()), @@ -264,7 +296,7 @@ pub fn apply_patch( stderr: &mut impl std::io::Write, ) -> Result<(), ApplyPatchError> { let hunks = match parse_patch(patch) { - Ok(hunks) => hunks, + Ok(source) => source.hunks, Err(e) => { match &e { InvalidPatchError(message) => { @@ -652,7 +684,7 @@ mod tests { ]); match maybe_parse_apply_patch(&args) { - MaybeApplyPatch::Body(hunks) => { + MaybeApplyPatch::Body(ApplyPatchSource { hunks, patch: _ }) => { assert_eq!( hunks, vec![Hunk::AddFile { @@ -679,7 +711,7 @@ PATCH"#, ]); match maybe_parse_apply_patch(&args) { - MaybeApplyPatch::Body(hunks) => { + MaybeApplyPatch::Body(ApplyPatchSource { hunks, patch: _ }) => { assert_eq!( hunks, vec![Hunk::AddFile { @@ -954,7 +986,7 @@ PATCH"#, )); let patch = parse_patch(&patch).unwrap(); - let update_file_chunks = match patch.as_slice() { + let update_file_chunks = match patch.hunks.as_slice() { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; @@ -992,7 +1024,7 @@ PATCH"#, )); let patch = parse_patch(&patch).unwrap(); - let chunks = match patch.as_slice() { + let chunks = match patch.hunks.as_slice() { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; @@ -1029,7 +1061,7 @@ PATCH"#, )); let patch = parse_patch(&patch).unwrap(); - let chunks = match patch.as_slice() { + let chunks = match patch.hunks.as_slice() { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; @@ -1064,7 +1096,7 @@ PATCH"#, )); let patch = parse_patch(&patch).unwrap(); - let chunks = match patch.as_slice() { + let chunks = match patch.hunks.as_slice() { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; @@ -1110,7 +1142,7 @@ PATCH"#, // Extract chunks then build the unified diff. let parsed = parse_patch(&patch).unwrap(); - let chunks = match parsed.as_slice() { + let chunks = match parsed.hunks.as_slice() { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; @@ -1193,6 +1225,8 @@ g new_content: "updated session directory content\n".to_string(), }, )]), + patch: argv[1].clone(), + cwd: session_dir.path().to_path_buf(), }) ); } diff --git a/codex-rs/apply-patch/src/parser.rs b/codex-rs/apply-patch/src/parser.rs index d07691a49d..edd622728e 100644 --- a/codex-rs/apply-patch/src/parser.rs +++ b/codex-rs/apply-patch/src/parser.rs @@ -86,6 +86,8 @@ impl Hunk { use Hunk::*; +use crate::ApplyPatchSource; + #[derive(Debug, PartialEq, Clone)] pub struct UpdateFileChunk { /// A single line of context used to narrow down the position of the chunk @@ -102,7 +104,7 @@ pub struct UpdateFileChunk { pub is_end_of_file: bool, } -pub fn parse_patch(patch: &str) -> Result, ParseError> { +pub fn parse_patch(patch: &str) -> Result { let mode = if PARSE_IN_STRICT_MODE { ParseMode::Strict } else { @@ -150,7 +152,7 @@ enum ParseMode { Lenient, } -fn parse_patch_text(patch: &str, mode: ParseMode) -> Result, ParseError> { +fn parse_patch_text(patch: &str, mode: ParseMode) -> Result { let lines: Vec<&str> = patch.trim().lines().collect(); let lines: &[&str] = match check_patch_boundaries_strict(&lines) { Ok(()) => &lines, @@ -173,7 +175,8 @@ fn parse_patch_text(patch: &str, mode: ParseMode) -> Result, ParseErro line_number += hunk_lines; remaining_lines = &remaining_lines[hunk_lines..] } - Ok(hunks) + let patch = lines.join("\n"); + Ok(ApplyPatchSource { hunks, patch }) } /// Checks the start and end lines of the patch text for `apply_patch`, @@ -455,8 +458,10 @@ fn test_parse_patch() { "*** Begin Patch\n\ *** End Patch", ParseMode::Strict - ), - Ok(Vec::new()) + ) + .unwrap() + .hunks, + Vec::new() ); assert_eq!( parse_patch_text( @@ -472,8 +477,10 @@ fn test_parse_patch() { + return 123\n\ *** End Patch", ParseMode::Strict - ), - Ok(vec![ + ) + .unwrap() + .hunks, + vec![ AddFile { path: PathBuf::from("path/add.py"), contents: "abc\ndef\n".to_string() @@ -491,7 +498,7 @@ fn test_parse_patch() { is_end_of_file: false }] } - ]) + ] ); // Update hunk followed by another hunk (Add File). assert_eq!( @@ -504,8 +511,10 @@ fn test_parse_patch() { +content\n\ *** End Patch", ParseMode::Strict - ), - Ok(vec![ + ) + .unwrap() + .hunks, + vec![ UpdateFile { path: PathBuf::from("file.py"), move_path: None, @@ -520,7 +529,7 @@ fn test_parse_patch() { path: PathBuf::from("other.py"), contents: "content\n".to_string() } - ]) + ] ); // Update hunk without an explicit @@ header for the first chunk should parse. @@ -533,8 +542,10 @@ fn test_parse_patch() { +bar *** End Patch"#, ParseMode::Strict - ), - Ok(vec![UpdateFile { + ) + .unwrap() + .hunks, + vec![UpdateFile { path: PathBuf::from("file2.py"), move_path: None, chunks: vec![UpdateFileChunk { @@ -543,7 +554,7 @@ fn test_parse_patch() { new_lines: vec!["import foo".to_string(), "bar".to_string()], is_end_of_file: false, }], - }]) + }] ); } @@ -574,7 +585,10 @@ fn test_parse_patch_lenient() { ); assert_eq!( parse_patch_text(&patch_text_in_heredoc, ParseMode::Lenient), - Ok(expected_patch.clone()) + Ok(ApplyPatchSource { + hunks: expected_patch.clone(), + patch: patch_text.to_string() + }) ); let patch_text_in_single_quoted_heredoc = format!("<<'EOF'\n{patch_text}\nEOF\n"); @@ -584,7 +598,10 @@ fn test_parse_patch_lenient() { ); assert_eq!( parse_patch_text(&patch_text_in_single_quoted_heredoc, ParseMode::Lenient), - Ok(expected_patch.clone()) + Ok(ApplyPatchSource { + hunks: expected_patch.clone(), + patch: patch_text.to_string() + }) ); let patch_text_in_double_quoted_heredoc = format!("<<\"EOF\"\n{patch_text}\nEOF\n"); @@ -594,7 +611,10 @@ fn test_parse_patch_lenient() { ); assert_eq!( parse_patch_text(&patch_text_in_double_quoted_heredoc, ParseMode::Lenient), - Ok(expected_patch.clone()) + Ok(ApplyPatchSource { + hunks: expected_patch.clone(), + patch: patch_text.to_string() + }) ); let patch_text_in_mismatched_quotes_heredoc = format!("<<\"EOF'\n{patch_text}\nEOF\n"); diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 44af72c746..2c05dba87f 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -18,12 +18,23 @@ use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; +pub(crate) enum InternalApplyPatchInvocation { + Output(ResponseInputItem), + DelegateToExec(ApplyPatchAction), +} + +impl From for InternalApplyPatchInvocation { + fn from(item: ResponseInputItem) -> Self { + InternalApplyPatchInvocation::Output(item) + } +} + pub(crate) async fn apply_patch( sess: &Session, - sub_id: String, - call_id: String, + sub_id: &str, + call_id: &str, action: ApplyPatchAction, -) -> ResponseInputItem { +) -> InternalApplyPatchInvocation { let writable_roots_snapshot = { #[allow(clippy::unwrap_used)] let guard = sess.writable_roots.lock().unwrap(); @@ -36,34 +47,38 @@ pub(crate) async fn apply_patch( &writable_roots_snapshot, &sess.cwd, ) { - SafetyCheck::AutoApprove { .. } => true, + SafetyCheck::AutoApprove { .. } => { + return InternalApplyPatchInvocation::DelegateToExec(action); + } SafetyCheck::AskUser => { // Compute a readable summary of path changes to include in the // approval request so the user can make an informed decision. let rx_approve = sess - .request_patch_approval(sub_id.clone(), call_id.clone(), &action, None, None) + .request_patch_approval(sub_id.to_owned(), call_id.to_owned(), &action, None, None) .await; match rx_approve.await.unwrap_or_default() { ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, ReviewDecision::Denied | ReviewDecision::Abort => { return ResponseInputItem::FunctionCallOutput { - call_id, + call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: "patch rejected by user".to_string(), success: Some(false), }, - }; + } + .into(); } } } SafetyCheck::Reject { reason } => { return ResponseInputItem::FunctionCallOutput { - call_id, + call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: format!("patch rejected: {reason}"), success: Some(false), }, - }; + } + .into(); } }; @@ -83,8 +98,8 @@ pub(crate) async fn apply_patch( let rx = sess .request_patch_approval( - sub_id.clone(), - call_id.clone(), + sub_id.to_owned(), + call_id.to_owned(), &action, reason.clone(), Some(root.clone()), @@ -96,12 +111,13 @@ pub(crate) async fn apply_patch( ReviewDecision::Approved | ReviewDecision::ApprovedForSession ) { return ResponseInputItem::FunctionCallOutput { - call_id, + call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: "patch rejected by user".to_string(), success: Some(false), }, - }; + } + .into(); } // user approved, extend writable roots for this session @@ -112,9 +128,9 @@ pub(crate) async fn apply_patch( let _ = sess .tx_event .send(Event { - id: sub_id.clone(), + id: sub_id.to_owned(), msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { - call_id: call_id.clone(), + call_id: call_id.to_owned(), auto_approved, changes: convert_apply_patch_to_protocol(&action), }), @@ -173,8 +189,8 @@ pub(crate) async fn apply_patch( )); let rx = sess .request_patch_approval( - sub_id.clone(), - call_id.clone(), + sub_id.to_owned(), + call_id.to_owned(), &action, reason.clone(), Some(root.clone()), @@ -204,9 +220,9 @@ pub(crate) async fn apply_patch( let _ = sess .tx_event .send(Event { - id: sub_id.clone(), + id: sub_id.to_owned(), msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { - call_id: call_id.clone(), + call_id: call_id.to_owned(), stdout: String::from_utf8_lossy(&stdout).to_string(), stderr: String::from_utf8_lossy(&stderr).to_string(), success: success_flag, @@ -214,22 +230,23 @@ pub(crate) async fn apply_patch( }) .await; - match result { + let item = match result { Ok(_) => ResponseInputItem::FunctionCallOutput { - call_id, + call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: String::from_utf8_lossy(&stdout).to_string(), success: None, }, }, Err(e) => ResponseInputItem::FunctionCallOutput { - call_id, + call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: format!("error: {e:#}, stderr: {}", String::from_utf8_lossy(&stderr)), success: Some(false), }, }, - } + }; + InternalApplyPatchInvocation::Output(item) } /// Return the first path in `hunks` that is NOT under any of the diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 3ab3e8d780..c9eabc5d13 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -29,6 +29,7 @@ use tracing::trace; use tracing::warn; use uuid::Uuid; +use crate::apply_patch::InternalApplyPatchInvocation; use crate::apply_patch::convert_apply_patch_to_protocol; use crate::apply_patch::get_writable_roots; use crate::apply_patch::{self}; @@ -1411,82 +1412,118 @@ async fn handle_container_exec_with_params( call_id: String, ) -> ResponseInputItem { // check if this was a patch, and apply it if so - match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { - MaybeApplyPatchVerified::Body(changes) => { - return apply_patch::apply_patch(sess, sub_id, call_id, changes).await; - } - MaybeApplyPatchVerified::CorrectnessError(parse_error) => { - // It looks like an invocation of `apply_patch`, but we - // could not resolve it into a patch that would apply - // cleanly. Return to model for resample. - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("error: {parse_error:#}"), - success: None, - }, - }; - } - MaybeApplyPatchVerified::ShellParseError(error) => { - trace!("Failed to parse shell command, {error:?}"); - } - MaybeApplyPatchVerified::NotApplyPatch => (), - } - - // safety checks - let safety = { - let state = sess.state.lock().unwrap(); - assess_command_safety( - ¶ms.command, - sess.approval_policy, - &sess.sandbox_policy, - &state.approved_commands, - ) - }; - let sandbox_type = match safety { - SafetyCheck::AutoApprove { sandbox_type } => sandbox_type, - SafetyCheck::AskUser => { - let rx_approve = sess - .request_command_approval( - sub_id.clone(), - call_id.clone(), - params.command.clone(), - params.cwd.clone(), - None, - ) - .await; - match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved => (), - ReviewDecision::ApprovedForSession => { - sess.add_approved_command(params.command.clone()); + let apply_patch_action_for_exec = + match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { + MaybeApplyPatchVerified::Body(changes) => { + match apply_patch::apply_patch(sess, &sub_id, &call_id, changes).await { + InternalApplyPatchInvocation::Output(item) => return item, + InternalApplyPatchInvocation::DelegateToExec(action) => Some(action), } - ReviewDecision::Denied | ReviewDecision::Abort => { + } + MaybeApplyPatchVerified::CorrectnessError(parse_error) => { + // It looks like an invocation of `apply_patch`, but we + // could not resolve it into a patch that would apply + // cleanly. Return to model for resample. + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!("error: {parse_error:#}"), + success: None, + }, + }; + } + MaybeApplyPatchVerified::ShellParseError(error) => { + trace!("Failed to parse shell command, {error:?}"); + None + } + MaybeApplyPatchVerified::NotApplyPatch => None, + }; + + let (sandbox_type, params) = match apply_patch_action_for_exec { + Some(ApplyPatchAction { patch, cwd, .. }) => { + // If we are applying a patch, we do not run the command in a sandbox. + // Instead, we run it directly in the host environment. + ( + // TODO(mbolin): Need to get this from assess_command_safety()? + SandboxType::None, + ExecParams { + // TODO(mbolin): Do not blow up if current_exe is not UTF-8? + #[allow(clippy::unwrap_used)] + command: vec![ + std::env::current_exe() + .ok() + .unwrap() + .to_string_lossy() + .to_string(), + "--codex-run-as-apply-patch".to_string(), + patch, + ], + cwd, + timeout_ms: params.timeout_ms, + env: HashMap::new(), + }, + ) + } + None => { + // safety checks + let safety = { + let state = sess.state.lock().unwrap(); + assess_command_safety( + ¶ms.command, + sess.approval_policy, + &sess.sandbox_policy, + &state.approved_commands, + ) + }; + let sandbox_type = match safety { + SafetyCheck::AutoApprove { sandbox_type } => sandbox_type, + SafetyCheck::AskUser => { + let rx_approve = sess + .request_command_approval( + sub_id.clone(), + call_id.clone(), + params.command.clone(), + params.cwd.clone(), + None, + ) + .await; + match rx_approve.await.unwrap_or_default() { + ReviewDecision::Approved => (), + ReviewDecision::ApprovedForSession => { + sess.add_approved_command(params.command.clone()); + } + ReviewDecision::Denied | ReviewDecision::Abort => { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: "exec command rejected by user".to_string(), + success: None, + }, + }; + } + } + // No sandboxing is applied because the user has given + // explicit approval. Often, we end up in this case because + // the command cannot be run in a sandbox, such as + // installing a new dependency that requires network access. + SandboxType::None + } + SafetyCheck::Reject { reason } => { return ResponseInputItem::FunctionCallOutput { call_id, output: FunctionCallOutputPayload { - content: "exec command rejected by user".to_string(), + content: format!("exec command rejected: {reason}"), success: None, }, }; } - } - // No sandboxing is applied because the user has given - // explicit approval. Often, we end up in this case because - // the command cannot be run in a sandbox, such as - // installing a new dependency that requires network access. - SandboxType::None - } - SafetyCheck::Reject { reason } => { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("exec command rejected: {reason}"), - success: None, - }, }; + + (sandbox_type, params) } }; + // This will look funny for apply_patch? sess.notify_exec_command_begin(&sub_id, &call_id, ¶ms) .await;