From 35010812c7e8b40c3d921c6d3148db2a88d5a4a6 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 30 Jul 2025 17:49:07 -0700 Subject: [PATCH 1/8] chore: add support for a new label, codex-rust-review (#1744) The goal of this change is to try an experiment where we try to get AI to take on more of the code review load. The idea is that once you believe your PR is ready for review, please add the `codex-rust-review` label (as opposed to the `codex-review` label). Admittedly the corresponding prompt currently represents my personal biases in terms of code review, but we should massage it over time to represent the team's preferences. --- .github/codex/labels/codex-rust-review.md | 23 +++++++++++++++++++++++ .github/workflows/codex.yml | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 .github/codex/labels/codex-rust-review.md diff --git a/.github/codex/labels/codex-rust-review.md b/.github/codex/labels/codex-rust-review.md new file mode 100644 index 0000000000..2c2893a1fe --- /dev/null +++ b/.github/codex/labels/codex-rust-review.md @@ -0,0 +1,23 @@ +Review this PR and respond with a very concise final message, formatted in Markdown. + +There should be a summary of the changes (1-2 sentences) and a few bullet points if necessary. + +Then provide the **review** (1-2 sentences plus bullet points, friendly tone). + +Things to look out for when doing the review: + +- **Make sure the pull request body explains the motivation behind the change.** If the author has failed to do this, call it out, and if you think you can deduce the motivation behind the change, propose copy. +- Ideally, the PR body also contains a small summary of the change. For small changes, the PR title may be sufficient. +- Each PR should ideally do one conceptual thing. For example, if a PR does a refactoring as well as introducing a new feature, push back and suggest the refactoring be done in a separate PR. This makes things easier for the reviewer, as refactoring changes can often be far-reaching, yet quick to review. +- If the nature of the change seems to have a visual component (which is often the case for changes to `codex-rs/tui`), recommend including a screenshot or video to demonstrate the change, if appropriate. +- Rust files should generally be organized such that the public parts of the API appear near the top of the file and helper functions go below. This is analagous to the "inverted pyramid" structure that is favored in journalism. +- Encourage the use of small enums or the newtype pattern in Rust if it helps readability without adding significant cognitive load or lines of code. +- Be wary of large files and offer suggestions for how to break things into more reasonably-sized files. +- When modifying a `Cargo.toml` file, make sure that dependency lists stay alphabetically sorted. Also consider whether a new dependency is added to the appropriate place (e.g., `[dependencies]` versus `[dev-dependencies]`) +- If you see opportunities for the changes in a diff to use more idiomatic Rust, please make specific recommendations. For example, favor the use of expressions over `return`. +- When introducing new code, be on the lookout for code that duplicates existing code. When found, propose a way to refactor the existing code such that it should be reused. +- Each create in the Cargo workspace in `codex-rs` has a specific purpose: make a note if you believe new code is not introduced in the correct crate. +- When possible, try to keep the `core` crate as small as possible. Non-core but shared logic is often a good candidate for `codex-rs/common`. +- References to existing GitHub issues and PRs are encouraged, where appropriate, though you likely do not have network access, so may not be able to help here. + +{CODEX_ACTION_GITHUB_EVENT_PATH} contains the JSON that triggered this GitHub workflow. It contains the `base` and `head` refs that define this PR. Both refs are available locally. diff --git a/.github/workflows/codex.yml b/.github/workflows/codex.yml index a0ac5b9740..18fe74cc85 100644 --- a/.github/workflows/codex.yml +++ b/.github/workflows/codex.yml @@ -20,7 +20,7 @@ jobs: (github.event_name == 'issues' && ( (github.event.action == 'labeled' && (github.event.label.name == 'codex-attempt' || github.event.label.name == 'codex-triage')) )) || - (github.event_name == 'pull_request' && github.event.action == 'labeled' && github.event.label.name == 'codex-review') + (github.event_name == 'pull_request' && github.event.action == 'labeled' && (github.event.label.name == 'codex-review' || github.event.label.name == 'codex-rust-review')) runs-on: ubuntu-latest permissions: contents: write # can push or create branches From 51b6bdefbeb47adf9fb6f7fe4a03461376ab56a8 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 30 Jul 2025 18:37:00 -0700 Subject: [PATCH 2/8] Auto format toml (#1745) Add recommended extension and configure it to auto format prompt. --- .vscode/extensions.json | 5 +++++ .vscode/settings.json | 8 +++++++- codex-rs/Cargo.toml | 3 +-- codex-rs/ansi-escape/Cargo.toml | 4 ++-- codex-rs/apply-patch/Cargo.toml | 2 +- codex-rs/arg0/Cargo.toml | 2 +- codex-rs/chatgpt/Cargo.toml | 6 +++--- codex-rs/cli/Cargo.toml | 4 ++-- codex-rs/common/Cargo.toml | 6 +++--- codex-rs/core/Cargo.toml | 8 ++++---- codex-rs/exec/Cargo.toml | 4 ++-- codex-rs/file-search/Cargo.toml | 2 +- codex-rs/linux-sandbox/Cargo.toml | 4 ++-- codex-rs/login/Cargo.toml | 2 +- codex-rs/mcp-server/Cargo.toml | 10 +++++----- codex-rs/mcp-types/Cargo.toml | 2 +- codex-rs/tui/Cargo.toml | 4 ++-- 17 files changed, 43 insertions(+), 33 deletions(-) create mode 100644 .vscode/extensions.json diff --git a/.vscode/extensions.json b/.vscode/extensions.json new file mode 100644 index 0000000000..dd5dac527d --- /dev/null +++ b/.vscode/extensions.json @@ -0,0 +1,5 @@ +{ + "recommendations": [ + "tamasfe.even-better-toml", + ] +} diff --git a/.vscode/settings.json b/.vscode/settings.json index f66a12583c..1712f5989b 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -6,5 +6,11 @@ "[rust]": { "editor.defaultFormatter": "rust-lang.rust-analyzer", "editor.formatOnSave": true, - } + }, + "[toml]": { + "editor.defaultFormatter": "tamasfe.even-better-toml", + "editor.formatOnSave": true, + }, + "evenBetterToml.formatter.reorderArrays": true, + "evenBetterToml.formatter.reorderKeys": true, } diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 51b2b5cc12..0f8085c7e5 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -1,5 +1,4 @@ [workspace] -resolver = "2" members = [ "ansi-escape", "apply-patch", @@ -17,6 +16,7 @@ members = [ "mcp-types", "tui", ] +resolver = "2" [workspace.package] version = "0.0.0" @@ -45,4 +45,3 @@ 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/ansi-escape/Cargo.toml b/codex-rs/ansi-escape/Cargo.toml index 9092c77c9c..ada675380d 100644 --- a/codex-rs/ansi-escape/Cargo.toml +++ b/codex-rs/ansi-escape/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-ansi-escape" version = { workspace = true } -edition = "2024" [lib] name = "codex_ansi_escape" @@ -10,7 +10,7 @@ path = "src/lib.rs" [dependencies] ansi-to-tui = "7.0.0" ratatui = { version = "0.29.0", features = [ - "unstable-widget-ref", "unstable-rendered-line-info", + "unstable-widget-ref", ] } tracing = { version = "0.1.41", features = ["log"] } diff --git a/codex-rs/apply-patch/Cargo.toml b/codex-rs/apply-patch/Cargo.toml index 5b95d4fa15..622f53ce71 100644 --- a/codex-rs/apply-patch/Cargo.toml +++ b/codex-rs/apply-patch/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-apply-patch" version = { workspace = true } -edition = "2024" [lib] name = "codex_apply_patch" diff --git a/codex-rs/arg0/Cargo.toml b/codex-rs/arg0/Cargo.toml index 7c55ac0d96..d668ffeff9 100644 --- a/codex-rs/arg0/Cargo.toml +++ b/codex-rs/arg0/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-arg0" version = { workspace = true } -edition = "2024" [lib] name = "codex_arg0" diff --git a/codex-rs/chatgpt/Cargo.toml b/codex-rs/chatgpt/Cargo.toml index e07543f4e8..903dc14b51 100644 --- a/codex-rs/chatgpt/Cargo.toml +++ b/codex-rs/chatgpt/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-chatgpt" version = { workspace = true } -edition = "2024" [lints] workspace = true @@ -9,12 +9,12 @@ workspace = true [dependencies] anyhow = "1" clap = { version = "4", features = ["derive"] } -serde = { version = "1", features = ["derive"] } -serde_json = "1" codex-common = { path = "../common", features = ["cli"] } codex-core = { path = "../core" } codex-login = { path = "../login" } reqwest = { version = "0.12", features = ["json", "stream"] } +serde = { version = "1", features = ["derive"] } +serde_json = "1" tokio = { version = "1", features = ["full"] } [dev-dependencies] diff --git a/codex-rs/cli/Cargo.toml b/codex-rs/cli/Cargo.toml index ab98764bed..0f370691cf 100644 --- a/codex-rs/cli/Cargo.toml +++ b/codex-rs/cli/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-cli" version = { workspace = true } -edition = "2024" [[bin]] name = "codex" @@ -20,8 +20,8 @@ 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-core = { path = "../core" } codex-exec = { path = "../exec" } codex-login = { path = "../login" } codex-mcp-server = { path = "../mcp-server" } diff --git a/codex-rs/common/Cargo.toml b/codex-rs/common/Cargo.toml index 3b843181cf..1723098b8a 100644 --- a/codex-rs/common/Cargo.toml +++ b/codex-rs/common/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-common" version = { workspace = true } -edition = "2024" [lints] workspace = true @@ -9,11 +9,11 @@ workspace = true [dependencies] clap = { version = "4", features = ["derive", "wrap_help"], optional = true } codex-core = { path = "../core" } -toml = { version = "0.9", optional = true } serde = { version = "1", optional = true } +toml = { version = "0.9", optional = true } [features] # Separate feature so that `clap` is not a mandatory dependency. -cli = ["clap", "toml", "serde"] +cli = ["clap", "serde", "toml"] elapsed = [] sandbox_summary = [] diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 5ebb5ef63d..ecc904cd5e 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-core" version = { workspace = true } -edition = "2024" [lib] name = "codex_core" @@ -15,10 +15,10 @@ anyhow = "1" async-channel = "2.3.1" base64 = "0.22" bytes = "1.10.1" -codex-apply-patch = { path = "../apply-patch" } -codex-mcp-client = { path = "../mcp-client" } chrono = { version = "0.4", features = ["serde"] } +codex-apply-patch = { path = "../apply-patch" } codex-login = { path = "../login" } +codex-mcp-client = { path = "../mcp-client" } dirs = "6" env-flags = "0.1.1" eventsource-stream = "0.2.3" @@ -49,8 +49,8 @@ tracing = { version = "0.1.41", features = ["log"] } tree-sitter = "0.25.8" tree-sitter-bash = "0.25.0" uuid = { version = "1", features = ["serde", "v4"] } -wildmatch = "2.4.0" whoami = "1.6.0" +wildmatch = "2.4.0" [target.'cfg(target_os = "linux")'.dependencies] diff --git a/codex-rs/exec/Cargo.toml b/codex-rs/exec/Cargo.toml index ced771f238..cd521410b1 100644 --- a/codex-rs/exec/Cargo.toml +++ b/codex-rs/exec/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-exec" version = { workspace = true } -edition = "2024" [[bin]] name = "codex-exec" @@ -19,12 +19,12 @@ 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-core = { path = "../core" } owo-colors = "4.2.0" serde_json = "1" shlex = "1.3.0" diff --git a/codex-rs/file-search/Cargo.toml b/codex-rs/file-search/Cargo.toml index bb5b80b2cf..3f70377183 100644 --- a/codex-rs/file-search/Cargo.toml +++ b/codex-rs/file-search/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-file-search" version = { workspace = true } -edition = "2024" [[bin]] name = "codex-file-search" diff --git a/codex-rs/linux-sandbox/Cargo.toml b/codex-rs/linux-sandbox/Cargo.toml index 4b173ea17a..ea7052c409 100644 --- a/codex-rs/linux-sandbox/Cargo.toml +++ b/codex-rs/linux-sandbox/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-linux-sandbox" version = { workspace = true } -edition = "2024" [[bin]] name = "codex-linux-sandbox" @@ -19,8 +19,8 @@ anyhow = "1" clap = { version = "4", features = ["derive"] } codex-common = { path = "../common", features = ["cli"] } codex-core = { path = "../core" } -libc = "0.2.172" landlock = "0.4.1" +libc = "0.2.172" seccompiler = "0.5.0" [target.'cfg(target_os = "linux")'.dev-dependencies] diff --git a/codex-rs/login/Cargo.toml b/codex-rs/login/Cargo.toml index e6eba6fd4f..e10666b092 100644 --- a/codex-rs/login/Cargo.toml +++ b/codex-rs/login/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-login" version = { workspace = true } -edition = "2024" [lints] workspace = true diff --git a/codex-rs/mcp-server/Cargo.toml b/codex-rs/mcp-server/Cargo.toml index 19cf4db538..2f618808c1 100644 --- a/codex-rs/mcp-server/Cargo.toml +++ b/codex-rs/mcp-server/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-mcp-server" version = { workspace = true } -edition = "2024" [[bin]] name = "codex-mcp-server" @@ -23,9 +23,7 @@ schemars = "0.8.22" serde = { version = "1", features = ["derive"] } serde_json = "1" shlex = "1.3.0" -toml = "0.9" -tracing = { version = "0.1.41", features = ["log"] } -tracing-subscriber = { version = "0.3", features = ["fmt", "env-filter"] } +strum_macros = "0.27.2" tokio = { version = "1", features = [ "io-std", "macros", @@ -33,8 +31,10 @@ tokio = { version = "1", features = [ "rt-multi-thread", "signal", ] } +toml = "0.9" +tracing = { version = "0.1.41", features = ["log"] } +tracing-subscriber = { version = "0.3", features = ["env-filter", "fmt"] } uuid = { version = "1", features = ["serde", "v4"] } -strum_macros = "0.27.2" [dev-dependencies] assert_cmd = "2" diff --git a/codex-rs/mcp-types/Cargo.toml b/codex-rs/mcp-types/Cargo.toml index 81ac2d9761..db849d5f0e 100644 --- a/codex-rs/mcp-types/Cargo.toml +++ b/codex-rs/mcp-types/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "mcp-types" version = { workspace = true } -edition = "2024" [lints] workspace = true diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 2f150921fb..63d287ca11 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -1,7 +1,7 @@ [package] +edition = "2024" name = "codex-tui" version = { workspace = true } -edition = "2024" [[bin]] name = "codex-tui" @@ -20,12 +20,12 @@ 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", "elapsed", "sandbox_summary", ] } +codex-core = { path = "../core" } codex-file-search = { path = "../file-search" } codex-login = { path = "../login" } color-eyre = "0.6.3" From defeafb279d3f58f38e64d7c7c855c9edf7a5cf1 Mon Sep 17 00:00:00 2001 From: pap-openai Date: Thu, 31 Jul 2025 04:23:56 +0100 Subject: [PATCH 3/8] add keyboard enhancements to support shift_return (#1743) For terminal that supports [keyboard enhancements](https://docs.rs/libcrossterm/latest/crossterm/enum.KeyboardEnhancementFlags.html), adds the enhancements (enabling [kitty keyboard protocol](https://sw.kovidgoyal.net/kitty/keyboard-protocol/)) to support shift+enter listener. Those users (users with terminals listed on [KPP](https://sw.kovidgoyal.net/kitty/keyboard-protocol/)) should be able to press shift+return for new line --------- Co-authored-by: easong-openai --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 14 ++++++++++++++ codex-rs/tui/src/tui.rs | 15 +++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 4d313f14a5..9c52057cc5 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -469,6 +469,20 @@ impl ChatComposer<'_> { self.textarea.insert_newline(); (InputResult::None, true) } + Input { + key: Key::Char('d'), + ctrl: true, + alt: false, + shift: false, + } => { + self.textarea.input(Input { + key: Key::Delete, + ctrl: false, + alt: false, + shift: false, + }); + (InputResult::None, true) + } input => self.handle_input_basic(input), } } diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index 1b215961ab..268483cbcf 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -5,6 +5,9 @@ use std::io::stdout; use codex_core::config::Config; use crossterm::event::DisableBracketedPaste; use crossterm::event::EnableBracketedPaste; +use crossterm::event::KeyboardEnhancementFlags; +use crossterm::event::PopKeyboardEnhancementFlags; +use crossterm::event::PushKeyboardEnhancementFlags; use ratatui::backend::CrosstermBackend; use ratatui::crossterm::execute; use ratatui::crossterm::terminal::disable_raw_mode; @@ -20,6 +23,17 @@ pub fn init(_config: &Config) -> Result { execute!(stdout(), EnableBracketedPaste)?; enable_raw_mode()?; + // Enable keyboard enhancement flags so modifiers for keys like Enter are disambiguated. + // chat_composer.rs is using a keyboard event listener to enter for any modified keys + // to create a new line that require this. + execute!( + stdout(), + PushKeyboardEnhancementFlags( + KeyboardEnhancementFlags::DISAMBIGUATE_ESCAPE_CODES + | KeyboardEnhancementFlags::REPORT_EVENT_TYPES + | KeyboardEnhancementFlags::REPORT_ALTERNATE_KEYS + ) + )?; set_panic_hook(); let backend = CrosstermBackend::new(stdout()); @@ -37,6 +51,7 @@ fn set_panic_hook() { /// Restore the terminal to its original state pub fn restore() -> Result<()> { + execute!(stdout(), PopKeyboardEnhancementFlags)?; execute!(stdout(), DisableBracketedPaste)?; disable_raw_mode()?; Ok(()) From d86270696e7bda0c321c7e981edbc7562ad3d330 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 31 Jul 2025 00:43:21 -0700 Subject: [PATCH 4/8] streamline ui (#1733) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify and improve many UI elements. * Remove all-around borders in most places. These interact badly with terminal resizing and look heavy. Prefer left-side-only borders. * Make the viewport adjust to the size of its contents. * / and @ autocomplete boxes appear below the prompt, instead of above it. * Restyle the keyboard shortcut hints & move them to the left. * Restyle the approval dialog. * Use synchronized rendering to avoid flashing during rerenders. https://github.com/user-attachments/assets/96f044af-283b-411c-b7fc-5e6b8a433c20 Screenshot 2025-07-30 at 5 29 20 PM --- codex-rs/tui/src/app.rs | 21 +++- .../src/bottom_pane/approval_modal_view.rs | 4 + .../tui/src/bottom_pane/bottom_pane_view.rs | 3 + codex-rs/tui/src/bottom_pane/chat_composer.rs | 100 ++++++++++-------- codex-rs/tui/src/bottom_pane/command_popup.rs | 48 ++++----- .../tui/src/bottom_pane/file_search_popup.rs | 28 +++-- codex-rs/tui/src/bottom_pane/mod.rs | 7 +- ...mposer__tests__backspace_after_pastes.snap | 20 ++-- ...tom_pane__chat_composer__tests__empty.snap | 20 ++-- ...tom_pane__chat_composer__tests__large.snap | 20 ++-- ...chat_composer__tests__multiple_pastes.snap | 20 ++-- ...tom_pane__chat_composer__tests__small.snap | 20 ++-- .../src/bottom_pane/status_indicator_view.rs | 4 + codex-rs/tui/src/chatwidget.rs | 32 ++++-- codex-rs/tui/src/history_cell.rs | 9 +- codex-rs/tui/src/slash_command.rs | 4 + codex-rs/tui/src/status_indicator_widget.rs | 8 +- codex-rs/tui/src/user_approval_widget.rs | 37 ++++--- 18 files changed, 232 insertions(+), 173 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 13ceabd7aa..ad4bc24fd4 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -9,7 +9,10 @@ use crate::slash_command::SlashCommand; use crate::tui; use codex_core::config::Config; use codex_core::protocol::Event; +use codex_core::protocol::EventMsg; +use codex_core::protocol::ExecApprovalRequestEvent; use color_eyre::eyre::Result; +use crossterm::SynchronizedUpdate; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use ratatui::layout::Offset; @@ -201,7 +204,7 @@ impl App<'_> { self.schedule_redraw(); } AppEvent::Redraw => { - self.draw_next_frame(terminal)?; + std::io::stdout().sync_update(|_| self.draw_next_frame(terminal))??; } AppEvent::KeyEvent(key_event) => { match key_event { @@ -297,6 +300,18 @@ impl App<'_> { widget.add_diff_output(text); } } + #[cfg(debug_assertions)] + SlashCommand::TestApproval => { + self.app_event_tx.send(AppEvent::CodexEvent(Event { + id: "1".to_string(), + msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { + call_id: "1".to_string(), + command: vec!["git".into(), "apply".into()], + cwd: self.config.cwd.clone(), + reason: Some("test".to_string()), + }), + })); + } }, AppEvent::StartFileSearch(query) => { self.file_search.on_user_query(query); @@ -321,8 +336,6 @@ impl App<'_> { } fn draw_next_frame(&mut self, terminal: &mut tui::Tui) -> Result<()> { - // TODO: add a throttle to avoid redrawing too often - let screen_size = terminal.size()?; let last_known_screen_size = terminal.last_known_screen_size; if screen_size != last_known_screen_size { @@ -345,7 +358,7 @@ impl App<'_> { let size = terminal.size()?; let desired_height = match &self.app_state { - AppState::Chat { widget } => widget.desired_height(), + AppState::Chat { widget } => widget.desired_height(size.width), AppState::GitWarning { .. } => 10, }; let mut area = terminal.viewport_area; diff --git a/codex-rs/tui/src/bottom_pane/approval_modal_view.rs b/codex-rs/tui/src/bottom_pane/approval_modal_view.rs index 376135ef31..4cd952f9eb 100644 --- a/codex-rs/tui/src/bottom_pane/approval_modal_view.rs +++ b/codex-rs/tui/src/bottom_pane/approval_modal_view.rs @@ -57,6 +57,10 @@ impl<'a> BottomPaneView<'a> for ApprovalModalView<'a> { self.current.is_complete() && self.queue.is_empty() } + fn desired_height(&self, width: u16) -> u16 { + self.current.desired_height(width) + } + fn render(&self, area: Rect, buf: &mut Buffer) { (&self.current).render_ref(area, buf); } diff --git a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs index 96922d94e7..a5616371d2 100644 --- a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs +++ b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs @@ -28,6 +28,9 @@ pub(crate) trait BottomPaneView<'a> { CancellationEvent::Ignored } + /// Return the desired height of the view. + fn desired_height(&self, width: u16) -> u16; + /// Render the view: this will be displayed in place of the composer. fn render(&self, area: Rect, buf: &mut Buffer); diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 9c52057cc5..3bc573a003 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -1,11 +1,13 @@ use codex_core::protocol::TokenUsage; use crossterm::event::KeyEvent; use ratatui::buffer::Buffer; -use ratatui::layout::Alignment; use ratatui::layout::Rect; +use ratatui::style::Color; use ratatui::style::Style; +use ratatui::style::Styled; use ratatui::style::Stylize; use ratatui::text::Line; +use ratatui::text::Span; use ratatui::widgets::BorderType; use ratatui::widgets::Borders; use ratatui::widgets::Widget; @@ -22,7 +24,7 @@ use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use codex_file_search::FileMatch; -const BASE_PLACEHOLDER_TEXT: &str = "send a message"; +const BASE_PLACEHOLDER_TEXT: &str = "..."; /// If the pasted content exceeds this number of characters, replace it with a /// placeholder in the UI. const LARGE_PASTE_CHAR_THRESHOLD: usize = 1000; @@ -72,9 +74,9 @@ impl ChatComposer<'_> { } pub fn desired_height(&self) -> u16 { - 2 + self.textarea.lines().len() as u16 + self.textarea.lines().len().max(1) as u16 + match &self.active_popup { - ActivePopup::None => 0u16, + ActivePopup::None => 1u16, ActivePopup::Command(c) => c.calculate_required_height(), ActivePopup::File(c) => c.calculate_required_height(), } @@ -635,37 +637,17 @@ impl ChatComposer<'_> { } fn update_border(&mut self, has_focus: bool) { - struct BlockState { - right_title: Line<'static>, - border_style: Style, - } - - let bs = if has_focus { - if self.ctrl_c_quit_hint { - BlockState { - right_title: Line::from("Ctrl+C to quit").alignment(Alignment::Right), - border_style: Style::default(), - } - } else { - BlockState { - right_title: Line::from("Enter to send | Ctrl+D to quit | Ctrl+J for newline") - .alignment(Alignment::Right), - border_style: Style::default(), - } - } + let border_style = if has_focus { + Style::default().fg(Color::Cyan) } else { - BlockState { - right_title: Line::from(""), - border_style: Style::default().dim(), - } + Style::default().dim() }; self.textarea.set_block( ratatui::widgets::Block::default() - .title_bottom(bs.right_title) - .borders(Borders::ALL) - .border_type(BorderType::Rounded) - .border_style(bs.border_style), + .borders(Borders::LEFT) + .border_type(BorderType::QuadrantOutside) + .border_style(border_style), ); } } @@ -677,19 +659,19 @@ impl WidgetRef for &ChatComposer<'_> { let popup_height = popup.calculate_required_height(); // Split the provided rect so that the popup is rendered at the - // *top* and the textarea occupies the remaining space below. - let popup_rect = Rect { + // **bottom** and the textarea occupies the remaining space above. + let popup_height = popup_height.min(area.height); + let textarea_rect = Rect { x: area.x, y: area.y, width: area.width, - height: popup_height.min(area.height), + height: area.height.saturating_sub(popup_height), }; - - let textarea_rect = Rect { + let popup_rect = Rect { x: area.x, - y: area.y + popup_rect.height, + y: area.y + textarea_rect.height, width: area.width, - height: area.height.saturating_sub(popup_rect.height), + height: popup_height, }; popup.render(popup_rect, buf); @@ -698,25 +680,51 @@ impl WidgetRef for &ChatComposer<'_> { ActivePopup::File(popup) => { let popup_height = popup.calculate_required_height(); - let popup_rect = Rect { + let popup_height = popup_height.min(area.height); + let textarea_rect = Rect { x: area.x, y: area.y, width: area.width, - height: popup_height.min(area.height), - }; - - let textarea_rect = Rect { - x: area.x, - y: area.y + popup_rect.height, - width: area.width, height: area.height.saturating_sub(popup_height), }; + let popup_rect = Rect { + x: area.x, + y: area.y + textarea_rect.height, + width: area.width, + height: popup_height, + }; popup.render(popup_rect, buf); self.textarea.render(textarea_rect, buf); } ActivePopup::None => { - self.textarea.render(area, buf); + let mut textarea_rect = area; + textarea_rect.height = textarea_rect.height.saturating_sub(1); + self.textarea.render(textarea_rect, buf); + let mut bottom_line_rect = area; + bottom_line_rect.y += textarea_rect.height; + bottom_line_rect.height = 1; + let key_hint_style = Style::default().fg(Color::Cyan); + let hint = if self.ctrl_c_quit_hint { + vec![ + Span::from(" "), + "Ctrl+C again".set_style(key_hint_style), + Span::from(" to quit"), + ] + } else { + vec![ + Span::from(" "), + "⏎".set_style(key_hint_style), + Span::from(" send "), + "Shift+⏎".set_style(key_hint_style), + Span::from(" newline "), + "Ctrl+C".set_style(key_hint_style), + Span::from(" quit"), + ] + }; + Line::from(hint) + .style(Style::default().dim()) + .render_ref(bottom_line_rect, buf); } } } diff --git a/codex-rs/tui/src/bottom_pane/command_popup.rs b/codex-rs/tui/src/bottom_pane/command_popup.rs index da3b3a8253..364a8472dc 100644 --- a/codex-rs/tui/src/bottom_pane/command_popup.rs +++ b/codex-rs/tui/src/bottom_pane/command_popup.rs @@ -3,9 +3,9 @@ use ratatui::layout::Rect; use ratatui::style::Color; use ratatui::style::Style; use ratatui::style::Stylize; -use ratatui::widgets::Block; -use ratatui::widgets::BorderType; -use ratatui::widgets::Borders; +use ratatui::symbols::border::QUADRANT_LEFT_HALF; +use ratatui::text::Line; +use ratatui::text::Span; use ratatui::widgets::Cell; use ratatui::widgets::Row; use ratatui::widgets::Table; @@ -72,11 +72,7 @@ impl CommandPopup { /// rows required to show **at most** `MAX_POPUP_ROWS` commands plus the /// table/border overhead (one line at the top and one at the bottom). pub(crate) fn calculate_required_height(&self) -> u16 { - let matches = self.filtered_commands(); - let row_count = matches.len().clamp(1, MAX_POPUP_ROWS) as u16; - // Account for the border added by the Block that wraps the table. - // 2 = one line at the top, one at the bottom. - row_count + 2 + self.filtered_commands().len().clamp(1, MAX_POPUP_ROWS) as u16 } /// Return the list of commands that match the current filter. Matching is @@ -158,18 +154,19 @@ impl WidgetRef for CommandPopup { let default_style = Style::default(); let command_style = Style::default().fg(Color::LightBlue); for (idx, cmd) in visible_matches.iter().enumerate() { - let (cmd_style, desc_style) = if Some(idx) == self.selected_idx { - ( - command_style.bg(Color::DarkGray), - default_style.bg(Color::DarkGray), - ) - } else { - (command_style, default_style) - }; - rows.push(Row::new(vec![ - Cell::from(format!("/{}", cmd.command())).style(cmd_style), - Cell::from(cmd.description().to_string()).style(desc_style), + Cell::from(Line::from(vec![ + if Some(idx) == self.selected_idx { + Span::styled( + "›", + Style::default().bg(Color::DarkGray).fg(Color::LightCyan), + ) + } else { + Span::styled(QUADRANT_LEFT_HALF, Style::default().fg(Color::DarkGray)) + }, + Span::styled(format!("/{}", cmd.command()), command_style), + ])), + Cell::from(cmd.description().to_string()).style(default_style), ])); } } @@ -180,12 +177,13 @@ impl WidgetRef for CommandPopup { rows, [Constraint::Length(FIRST_COLUMN_WIDTH), Constraint::Min(10)], ) - .column_spacing(0) - .block( - Block::default() - .borders(Borders::ALL) - .border_type(BorderType::Rounded), - ); + .column_spacing(0); + // .block( + // Block::default() + // .borders(Borders::LEFT) + // .border_type(BorderType::QuadrantOutside) + // .border_style(Style::default().fg(Color::DarkGray)), + // ); table.render(area, buf); } diff --git a/codex-rs/tui/src/bottom_pane/file_search_popup.rs b/codex-rs/tui/src/bottom_pane/file_search_popup.rs index e15f8690ae..ac6c91cf47 100644 --- a/codex-rs/tui/src/bottom_pane/file_search_popup.rs +++ b/codex-rs/tui/src/bottom_pane/file_search_popup.rs @@ -115,12 +115,8 @@ impl FileSearchPopup { // row so the popup is still visible. When matches are present we show // up to MAX_RESULTS regardless of the waiting flag so the list // remains stable while a newer search is in-flight. - let rows = if self.matches.is_empty() { - 1 - } else { - self.matches.len().clamp(1, MAX_RESULTS) - } as u16; - rows + 2 // border + + self.matches.len().clamp(1, MAX_RESULTS) as u16 } } @@ -128,7 +124,14 @@ impl WidgetRef for &FileSearchPopup { fn render_ref(&self, area: Rect, buf: &mut Buffer) { // Prepare rows. let rows: Vec = if self.matches.is_empty() { - vec![Row::new(vec![Cell::from(" no matches ")])] + vec![Row::new(vec![ + Cell::from(if self.waiting { + "(searching …)" + } else { + "no matches" + }) + .style(Style::new().add_modifier(Modifier::ITALIC | Modifier::DIM)), + ])] } else { self.matches .iter() @@ -169,17 +172,12 @@ impl WidgetRef for &FileSearchPopup { .collect() }; - let mut title = format!(" @{} ", self.pending_query); - if self.waiting { - title.push_str(" (searching …)"); - } - let table = Table::new(rows, vec![Constraint::Percentage(100)]) .block( Block::default() - .borders(Borders::ALL) - .border_type(BorderType::Rounded) - .title(title), + .borders(Borders::LEFT) + .border_type(BorderType::QuadrantOutside) + .border_style(Style::default().fg(Color::DarkGray)), ) .widths([Constraint::Percentage(100)]); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 2ca858d8ce..2710a3e997 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -64,8 +64,11 @@ impl BottomPane<'_> { } } - pub fn desired_height(&self) -> u16 { - self.composer.desired_height() + pub fn desired_height(&self, width: u16) -> u16 { + self.active_view + .as_ref() + .map(|v| v.desired_height(width)) + .unwrap_or(self.composer.desired_height()) } /// Forward a key event to the active view or the composer. diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap index fa604c862b..4f155dab30 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap @@ -2,13 +2,13 @@ source: tui/src/bottom_pane/chat_composer.rs expression: terminal.backend() --- -"╭──────────────────────────────────────────────────────────────────────────────────────────────────╮" -"│[Pasted Content 1002 chars][Pasted Content 1004 chars] │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"╰───────────────────────────────────────────────Enter to send | Ctrl+D to quit | Ctrl+J for newline╯" +"▌[Pasted Content 1002 chars][Pasted Content 1004 chars] " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +" ⏎ send Shift+⏎ newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap index a89076d8aa..4e8371f177 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap @@ -2,13 +2,13 @@ source: tui/src/bottom_pane/chat_composer.rs expression: terminal.backend() --- -"╭──────────────────────────────────────────────────────────────────────────────────────────────────╮" -"│ send a message │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"╰───────────────────────────────────────────────Enter to send | Ctrl+D to quit | Ctrl+J for newline╯" +"▌ ... " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +" ⏎ send Shift+⏎ newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap index 39a62da400..80fea40d5f 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap @@ -2,13 +2,13 @@ source: tui/src/bottom_pane/chat_composer.rs expression: terminal.backend() --- -"╭──────────────────────────────────────────────────────────────────────────────────────────────────╮" -"│[Pasted Content 1005 chars] │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"╰───────────────────────────────────────────────Enter to send | Ctrl+D to quit | Ctrl+J for newline╯" +"▌[Pasted Content 1005 chars] " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +" ⏎ send Shift+⏎ newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap index cd94095431..26e8d26733 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap @@ -2,13 +2,13 @@ source: tui/src/bottom_pane/chat_composer.rs expression: terminal.backend() --- -"╭──────────────────────────────────────────────────────────────────────────────────────────────────╮" -"│[Pasted Content 1003 chars][Pasted Content 1007 chars] another short paste │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"╰───────────────────────────────────────────────Enter to send | Ctrl+D to quit | Ctrl+J for newline╯" +"▌[Pasted Content 1003 chars][Pasted Content 1007 chars] another short paste " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +" ⏎ send Shift+⏎ newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap index e6b55e36d8..0f1b9e6426 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap @@ -2,13 +2,13 @@ source: tui/src/bottom_pane/chat_composer.rs expression: terminal.backend() --- -"╭──────────────────────────────────────────────────────────────────────────────────────────────────╮" -"│short │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"│ │" -"╰───────────────────────────────────────────────Enter to send | Ctrl+D to quit | Ctrl+J for newline╯" +"▌short " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +"▌ " +" ⏎ send Shift+⏎ newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/status_indicator_view.rs b/codex-rs/tui/src/bottom_pane/status_indicator_view.rs index f8c06ec5e5..a944271e45 100644 --- a/codex-rs/tui/src/bottom_pane/status_indicator_view.rs +++ b/codex-rs/tui/src/bottom_pane/status_indicator_view.rs @@ -33,6 +33,10 @@ impl BottomPaneView<'_> for StatusIndicatorView { true } + fn desired_height(&self, width: u16) -> u16 { + self.view.desired_height(width) + } + fn render(&self, area: ratatui::layout::Rect, buf: &mut Buffer) { self.view.render_ref(area, buf); } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 33e3ee11e4..3ee724e687 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1,3 +1,4 @@ +use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; @@ -44,6 +45,12 @@ use crate::history_cell::PatchEventType; use crate::user_approval_widget::ApprovalRequest; use codex_file_search::FileMatch; +struct RunningCommand { + command: Vec, + #[allow(dead_code)] + cwd: PathBuf, +} + pub(crate) struct ChatWidget<'a> { app_event_tx: AppEventSender, codex_op_tx: UnboundedSender, @@ -56,6 +63,7 @@ pub(crate) struct ChatWidget<'a> { // We wait for the final AgentMessage event and then emit the full text // at once into scrollback so the history contains a single message. answer_buffer: String, + running_commands: HashMap, } struct UserMessage { @@ -140,11 +148,12 @@ impl ChatWidget<'_> { token_usage: TokenUsage::default(), reasoning_buffer: String::new(), answer_buffer: String::new(), + running_commands: HashMap::new(), } } - pub fn desired_height(&self) -> u16 { - self.bottom_pane.desired_height() + pub fn desired_height(&self, width: u16) -> u16 { + self.bottom_pane.desired_height(width) } pub(crate) fn handle_key_event(&mut self, key_event: KeyEvent) { @@ -343,12 +352,18 @@ impl ChatWidget<'_> { self.request_redraw(); } EventMsg::ExecCommandBegin(ExecCommandBeginEvent { - call_id: _, + call_id, command, - cwd: _, + cwd, }) => { + self.running_commands.insert( + call_id, + RunningCommand { + command: command.clone(), + cwd: cwd.clone(), + }, + ); self.add_to_history(HistoryCell::new_active_exec_command(command)); - self.request_redraw(); } EventMsg::PatchApplyBegin(PatchApplyBeginEvent { call_id: _, @@ -361,7 +376,6 @@ impl ChatWidget<'_> { PatchEventType::ApplyBegin { auto_approved }, changes, )); - self.request_redraw(); } EventMsg::ExecCommandEnd(ExecCommandEndEvent { call_id, @@ -369,8 +383,9 @@ impl ChatWidget<'_> { stdout, stderr, }) => { + let cmd = self.running_commands.remove(&call_id); self.add_to_history(HistoryCell::new_completed_exec_command( - call_id, + cmd.map(|cmd| cmd.command).unwrap_or_else(|| vec![call_id]), CommandOutput { exit_code, stdout, @@ -384,7 +399,6 @@ impl ChatWidget<'_> { invocation, }) => { self.add_to_history(HistoryCell::new_active_mcp_tool_call(invocation)); - self.request_redraw(); } EventMsg::McpToolCallEnd(McpToolCallEndEvent { call_id: _, @@ -419,7 +433,6 @@ impl ChatWidget<'_> { } event => { self.add_to_history(HistoryCell::new_background_event(format!("{event:?}"))); - self.request_redraw(); } } } @@ -436,7 +449,6 @@ impl ChatWidget<'_> { pub(crate) fn add_diff_output(&mut self, diff_output: String) { self.add_to_history(HistoryCell::new_diff_output(diff_output.clone())); - self.request_redraw(); } /// Forward file-search results to the bottom pane. diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 04279a01f8..956a0cc7ef 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1,4 +1,4 @@ -use crate::exec_command::escape_command; +use crate::exec_command::strip_bash_lc_and_escape; use crate::markdown::append_markdown; use crate::text_block::TextBlock; use crate::text_formatting::format_and_truncate_tool_result; @@ -246,7 +246,7 @@ impl HistoryCell { } pub(crate) fn new_active_exec_command(command: Vec) -> Self { - let command_escaped = escape_command(&command); + let command_escaped = strip_bash_lc_and_escape(&command); let lines: Vec> = vec![ Line::from(vec!["command".magenta(), " running...".dim()]), @@ -259,7 +259,7 @@ impl HistoryCell { } } - pub(crate) fn new_completed_exec_command(command: String, output: CommandOutput) -> Self { + pub(crate) fn new_completed_exec_command(command: Vec, output: CommandOutput) -> Self { let CommandOutput { exit_code, stdout, @@ -283,7 +283,8 @@ impl HistoryCell { let src = if exit_code == 0 { stdout } else { stderr }; - lines.push(Line::from(format!("$ {command}"))); + let cmdline = strip_bash_lc_and_escape(&command); + lines.push(Line::from(format!("$ {cmdline}"))); let mut lines_iter = src.lines(); for raw in lines_iter.by_ref().take(TOOL_CALL_MAX_LINES) { lines.push(ansi_escape_line(raw).dim()); diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index 603eb721cd..7df1bcbdec 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -15,6 +15,8 @@ pub enum SlashCommand { New, Diff, Quit, + #[cfg(debug_assertions)] + TestApproval, } impl SlashCommand { @@ -26,6 +28,8 @@ impl SlashCommand { SlashCommand::Diff => { "Show git diff of the working directory (including untracked files)" } + #[cfg(debug_assertions)] + SlashCommand::TestApproval => "Test approval request", } } diff --git a/codex-rs/tui/src/status_indicator_widget.rs b/codex-rs/tui/src/status_indicator_widget.rs index 973ef09818..7e6d267481 100644 --- a/codex-rs/tui/src/status_indicator_widget.rs +++ b/codex-rs/tui/src/status_indicator_widget.rs @@ -73,6 +73,10 @@ impl StatusIndicatorWidget { } } + pub fn desired_height(&self, _width: u16) -> u16 { + 1 + } + /// Update the line that is displayed in the widget. pub(crate) fn update_text(&mut self, text: String) { self.text = text.replace(['\n', '\r'], " "); @@ -91,8 +95,8 @@ impl WidgetRef for StatusIndicatorWidget { let widget_style = Style::default(); let block = Block::default() .padding(Padding::new(1, 0, 0, 0)) - .borders(Borders::ALL) - .border_type(BorderType::Rounded) + .borders(Borders::LEFT) + .border_type(BorderType::QuadrantOutside) .border_style(widget_style.dim()); // Animated 3‑dot pattern inside brackets. The *active* dot is bold // white, the others are dim. diff --git a/codex-rs/tui/src/user_approval_widget.rs b/codex-rs/tui/src/user_approval_widget.rs index a161c2c399..855a7ea3db 100644 --- a/codex-rs/tui/src/user_approval_widget.rs +++ b/codex-rs/tui/src/user_approval_widget.rs @@ -17,13 +17,11 @@ use ratatui::layout::Rect; use ratatui::prelude::*; use ratatui::text::Line; use ratatui::text::Span; -use ratatui::widgets::Block; -use ratatui::widgets::BorderType; -use ratatui::widgets::Borders; use ratatui::widgets::List; use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; use ratatui::widgets::WidgetRef; +use ratatui::widgets::Wrap; use tui_input::Input; use tui_input::backend::crossterm::EventHandler; @@ -134,10 +132,9 @@ impl UserApprovalWidget<'_> { None => cwd.display().to_string(), }; let mut contents: Vec = vec![ - Line::from("Shell Command".bold()), - Line::from(""), Line::from(vec![ - format!("{cwd_str}$").dim(), + Span::from(cwd_str).dim(), + Span::from("$"), Span::from(format!(" {cmd}")), ]), Line::from(""), @@ -147,7 +144,7 @@ impl UserApprovalWidget<'_> { contents.push(Line::from("")); } contents.extend(vec![Line::from("Allow command?"), Line::from("")]); - Paragraph::new(contents) + Paragraph::new(contents).wrap(Wrap { trim: false }) } ApprovalRequest::ApplyPatch { reason, grant_root, .. @@ -313,21 +310,21 @@ impl UserApprovalWidget<'_> { pub(crate) fn is_complete(&self) -> bool { self.done } + + pub(crate) fn desired_height(&self, width: u16) -> u16 { + self.get_confirmation_prompt_height(width - 2) + SELECT_OPTIONS.len() as u16 + 2 + } } const PLAIN: Style = Style::new(); -const BLUE_FG: Style = Style::new().fg(Color::Blue); +const BLUE_FG: Style = Style::new().fg(Color::LightCyan); impl WidgetRef for &UserApprovalWidget<'_> { fn render_ref(&self, area: Rect, buf: &mut Buffer) { // Take the area, wrap it in a block with a border, and divide up the // remaining area into two chunks: one for the confirmation prompt and // one for the response. - let outer = Block::default() - .title("Review") - .borders(Borders::ALL) - .border_type(BorderType::Rounded); - let inner = outer.inner(area); + let inner = area.inner(Margin::new(0, 2)); // Determine how many rows we can allocate for the static confirmation // prompt while *always* keeping enough space for the interactive @@ -384,8 +381,18 @@ impl WidgetRef for &UserApprovalWidget<'_> { } }; - outer.render(area, buf); + let border = ("◢◤") + .repeat((area.width / 2).into()) + .fg(Color::LightYellow); + + border.render_ref(area, buf); + Paragraph::new(" Execution Request ".bold().black().on_light_yellow()) + .alignment(Alignment::Center) + .render_ref(area, buf); + self.confirmation_prompt.clone().render(prompt_chunk, buf); - Widget::render(List::new(lines), response_chunk, buf); + List::new(lines).render_ref(response_chunk, buf); + + border.render_ref(Rect::new(0, area.y + area.height - 1, area.width, 1), buf); } } From be0cd3430053441027654630ccd4db736235ecda Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 31 Jul 2025 09:17:59 -0700 Subject: [PATCH 5/8] fix git tests (#1747) the git tests were failing on my local machine due to gpg signing config in my ~/.gitconfig. tests should not be affected by ~/.gitconfig, so configure them to ignore it. --- codex-rs/chatgpt/tests/apply_command_e2e.rs | 9 +++++++++ codex-rs/core/src/git_info.rs | 9 +++++++++ codex-rs/core/tests/cli_stream.rs | 11 +++++++++++ 3 files changed, 29 insertions(+) diff --git a/codex-rs/chatgpt/tests/apply_command_e2e.rs b/codex-rs/chatgpt/tests/apply_command_e2e.rs index 45c33bedb4..f1a35e1521 100644 --- a/codex-rs/chatgpt/tests/apply_command_e2e.rs +++ b/codex-rs/chatgpt/tests/apply_command_e2e.rs @@ -10,8 +10,13 @@ use tokio::process::Command; async fn create_temp_git_repo() -> anyhow::Result { let temp_dir = TempDir::new()?; let repo_path = temp_dir.path(); + let envs = vec![ + ("GIT_CONFIG_GLOBAL", "/dev/null"), + ("GIT_CONFIG_NOSYSTEM", "1"), + ]; let output = Command::new("git") + .envs(envs.clone()) .args(["init"]) .current_dir(repo_path) .output() @@ -25,12 +30,14 @@ async fn create_temp_git_repo() -> anyhow::Result { } Command::new("git") + .envs(envs.clone()) .args(["config", "user.email", "test@example.com"]) .current_dir(repo_path) .output() .await?; Command::new("git") + .envs(envs.clone()) .args(["config", "user.name", "Test User"]) .current_dir(repo_path) .output() @@ -39,12 +46,14 @@ async fn create_temp_git_repo() -> anyhow::Result { std::fs::write(repo_path.join("README.md"), "# Test Repo\n")?; Command::new("git") + .envs(envs.clone()) .args(["add", "README.md"]) .current_dir(repo_path) .output() .await?; let output = Command::new("git") + .envs(envs.clone()) .args(["commit", "-m", "Initial commit"]) .current_dir(repo_path) .output() diff --git a/codex-rs/core/src/git_info.rs b/codex-rs/core/src/git_info.rs index cf959d32d1..f5dc016e66 100644 --- a/codex-rs/core/src/git_info.rs +++ b/codex-rs/core/src/git_info.rs @@ -111,9 +111,14 @@ mod tests { // Helper function to create a test git repository async fn create_test_git_repo(temp_dir: &TempDir) -> PathBuf { let repo_path = temp_dir.path().to_path_buf(); + let envs = vec![ + ("GIT_CONFIG_GLOBAL", "/dev/null"), + ("GIT_CONFIG_NOSYSTEM", "1"), + ]; // Initialize git repo Command::new("git") + .envs(envs.clone()) .args(["init"]) .current_dir(&repo_path) .output() @@ -122,6 +127,7 @@ mod tests { // Configure git user (required for commits) Command::new("git") + .envs(envs.clone()) .args(["config", "user.name", "Test User"]) .current_dir(&repo_path) .output() @@ -129,6 +135,7 @@ mod tests { .expect("Failed to set git user name"); Command::new("git") + .envs(envs.clone()) .args(["config", "user.email", "test@example.com"]) .current_dir(&repo_path) .output() @@ -140,6 +147,7 @@ mod tests { fs::write(&test_file, "test content").expect("Failed to write test file"); Command::new("git") + .envs(envs.clone()) .args(["add", "."]) .current_dir(&repo_path) .output() @@ -147,6 +155,7 @@ mod tests { .expect("Failed to add files"); Command::new("git") + .envs(envs.clone()) .args(["commit", "-m", "Initial commit"]) .current_dir(&repo_path) .output() diff --git a/codex-rs/core/tests/cli_stream.rs b/codex-rs/core/tests/cli_stream.rs index 0ab7bd0bb2..ee0377fc10 100644 --- a/codex-rs/core/tests/cli_stream.rs +++ b/codex-rs/core/tests/cli_stream.rs @@ -460,9 +460,14 @@ async fn integration_git_info_unit_test() { // 1. Create temp directory for git repo let temp_dir = TempDir::new().unwrap(); let git_repo = temp_dir.path().to_path_buf(); + let envs = vec![ + ("GIT_CONFIG_GLOBAL", "/dev/null"), + ("GIT_CONFIG_NOSYSTEM", "1"), + ]; // 2. Initialize a git repository with some content let init_output = std::process::Command::new("git") + .envs(envs.clone()) .args(["init"]) .current_dir(&git_repo) .output() @@ -471,12 +476,14 @@ async fn integration_git_info_unit_test() { // Configure git user (required for commits) std::process::Command::new("git") + .envs(envs.clone()) .args(["config", "user.name", "Integration Test"]) .current_dir(&git_repo) .output() .unwrap(); std::process::Command::new("git") + .envs(envs.clone()) .args(["config", "user.email", "test@example.com"]) .current_dir(&git_repo) .output() @@ -487,12 +494,14 @@ async fn integration_git_info_unit_test() { std::fs::write(&test_file, "integration test content").unwrap(); std::process::Command::new("git") + .envs(envs.clone()) .args(["add", "."]) .current_dir(&git_repo) .output() .unwrap(); let commit_output = std::process::Command::new("git") + .envs(envs.clone()) .args(["commit", "-m", "Integration test commit"]) .current_dir(&git_repo) .output() @@ -501,6 +510,7 @@ async fn integration_git_info_unit_test() { // Create a branch to test branch detection std::process::Command::new("git") + .envs(envs.clone()) .args(["checkout", "-b", "integration-test-branch"]) .current_dir(&git_repo) .output() @@ -508,6 +518,7 @@ async fn integration_git_info_unit_test() { // Add a remote to test repository URL detection std::process::Command::new("git") + .envs(envs.clone()) .args([ "remote", "add", From 861ba8640356139ba15eebb89abc164a7b86d97d Mon Sep 17 00:00:00 2001 From: easong-openai Date: Thu, 31 Jul 2025 09:19:08 -0700 Subject: [PATCH 6/8] Show error message after panic (#1752) Previously we were swallowing errors and silently exiting, which isn't great for helping users help us. --- codex-rs/tui/src/lib.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 351fab4df8..6b5fe7f7ae 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -176,9 +176,13 @@ fn run_ratatui_app( color_eyre::install()?; // Forward panic reports through tracing so they appear in the UI status - // line instead of interleaving raw panic output with the interface. - std::panic::set_hook(Box::new(|info| { + // line, but do not swallow the default/color-eyre panic handler. + // Chain to the previous hook so users still get a rich panic report + // (including backtraces) after we restore the terminal. + let prev_hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { tracing::error!("panic: {info}"); + prev_hook(info); })); let mut terminal = tui::init(&config)?; terminal.clear()?; From 96654a5d522fc885a19d38c1905087c7ae9b30b4 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 31 Jul 2025 09:59:36 -0700 Subject: [PATCH 7/8] clamp render area to terminal size (#1758) this fixes a couple of panics that would happen when trying to render something larger than the terminal, or insert history lines when the top of the viewport is at y=0. --- codex-rs/tui/src/app.rs | 2 +- codex-rs/tui/src/insert_history.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index ad4bc24fd4..44c1875d40 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -362,7 +362,7 @@ impl App<'_> { AppState::GitWarning { .. } => 10, }; let mut area = terminal.viewport_area; - area.height = desired_height; + area.height = desired_height.min(size.height); area.width = size.width; if area.bottom() > size.height { terminal diff --git a/codex-rs/tui/src/insert_history.rs b/codex-rs/tui/src/insert_history.rs index 54faf4beb8..efd08a71c2 100644 --- a/codex-rs/tui/src/insert_history.rs +++ b/codex-rs/tui/src/insert_history.rs @@ -36,12 +36,12 @@ pub(crate) fn insert_history_lines(terminal: &mut tui::Tui, lines: Vec) { .backend_mut() .scroll_region_down(area.top()..screen_size.height, scroll_amount) .ok(); - let cursor_top = area.top() - 1; + let cursor_top = area.top().saturating_sub(1); area.y += scroll_amount; terminal.set_viewport_area(area); cursor_top } else { - area.top() - 1 + area.top().saturating_sub(1) }; // Limit the scroll region to the lines from the top of the screen to the From 5b9ac442733aa74d5571bd03c6064b9728d42e44 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 31 Jul 2025 10:42:59 -0700 Subject: [PATCH 8/8] fix: ensure PatchApplyBeginEvent and PatchApplyEndEvent are dispatched reliably --- codex-rs/core/src/apply_patch.rs | 343 +++---------------------------- codex-rs/core/src/codex.rs | 226 +++++++++++++------- codex-rs/core/src/safety.rs | 12 +- 3 files changed, 193 insertions(+), 388 deletions(-) diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index f116c790ab..dc11aed023 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -1,19 +1,12 @@ 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; @@ -26,12 +19,18 @@ pub(crate) enum InternalApplyPatchInvocation { /// result to use with the `shell` function call that contained `apply_patch`. Output(ResponseInputItem), - /// The `apply_patch` call was auto-approved, which means that, on the - /// surface, it appears to be safe, but it should be run in a sandbox if the - /// user has configured one because a path being written could be a hard - /// link to a file outside the writable folders, so only the sandbox can - /// faithfully prevent the write in that case. - DelegateToExec(ApplyPatchAction), + /// The `apply_patch` call was approved, either automatically because it + /// appears that it should be allowed based on the user's sandbox policy + /// *or* because the user explicitly approved it. In either case, we use + /// exec with [`CODEX_APPLY_PATCH_ARG1`] to realize the `apply_patch` call, + /// but [`ApplyPatchExec::auto_approved`] is used to determine the sandbox + /// used with the `exec()`. + DelegateToExec(ApplyPatchExec), +} + +pub(crate) struct ApplyPatchExec { + pub(crate) action: ApplyPatchAction, + pub(crate) user_explicitly_approved_this_action: bool, } impl From for InternalApplyPatchInvocation { @@ -52,254 +51,57 @@ pub(crate) async fn apply_patch( guard.clone() }; - let auto_approved = match assess_patch_safety( + match assess_patch_safety( &action, sess.approval_policy, &writable_roots_snapshot, &sess.cwd, ) { SafetyCheck::AutoApprove { .. } => { - return InternalApplyPatchInvocation::DelegateToExec(action); + InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { + action, + user_explicitly_approved_this_action: false, + }) } SafetyCheck::AskUser => { // Compute a readable summary of path changes to include in the // approval request so the user can make an informed decision. + // + // Note that it might be worth expanding this approval request to + // give the user the option to expand the set of writable roots so + // that similar patches can be auto-approved in the future during + // this session. let rx_approve = sess .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::Approved | ReviewDecision::ApprovedForSession => { + InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { + action, + user_explicitly_approved_this_action: true, + }) + } ReviewDecision::Denied | ReviewDecision::Abort => { - return ResponseInputItem::FunctionCallOutput { + ResponseInputItem::FunctionCallOutput { call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: "patch rejected by user".to_string(), success: Some(false), }, } - .into(); + .into() } } } - SafetyCheck::Reject { reason } => { - return ResponseInputItem::FunctionCallOutput { - call_id: call_id.to_owned(), - output: FunctionCallOutputPayload { - content: format!("patch rejected: {reason}"), - success: Some(false), - }, - } - .into(); - } - }; - - // 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.to_owned(), - call_id.to_owned(), - &action, - reason.clone(), - Some(root.clone()), - ) - .await; - - if !matches!( - rx.await.unwrap_or_default(), - ReviewDecision::Approved | ReviewDecision::ApprovedForSession - ) { - return ResponseInputItem::FunctionCallOutput { - 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 - #[allow(clippy::unwrap_used)] - sess.writable_roots.lock().unwrap().push(root); - } - - let _ = sess - .tx_event - .send(Event { - id: sub_id.to_owned(), - msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { - call_id: call_id.to_owned(), - 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.to_owned(), - call_id.to_owned(), - &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.to_owned(), - msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { - 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, - }), - }) - .await; - - let item = match result { - Ok(_) => ResponseInputItem::FunctionCallOutput { + SafetyCheck::Reject { reason } => ResponseInputItem::FunctionCallOutput { 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.to_owned(), - output: FunctionCallOutputPayload { - content: format!("error: {e:#}, stderr: {}", String::from_utf8_lossy(&stderr)), + content: format!("patch rejected: {reason}"), success: Some(false), }, - }, - }; - InternalApplyPatchInvocation::Output(item) -} - -/// 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()); } + .into(), } - None } pub(crate) fn convert_apply_patch_to_protocol( @@ -327,85 +129,6 @@ pub(crate) fn convert_apply_patch_to_protocol( 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") { diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index cd92739c72..3dd1d513a2 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4,7 +4,6 @@ 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; @@ -31,6 +30,7 @@ use tracing::trace; use tracing::warn; use uuid::Uuid; +use crate::apply_patch::ApplyPatchExec; use crate::apply_patch::CODEX_APPLY_PATCH_ARG1; use crate::apply_patch::InternalApplyPatchInvocation; use crate::apply_patch::convert_apply_patch_to_protocol; @@ -74,8 +74,11 @@ 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; @@ -358,20 +361,32 @@ impl Session { } } - async fn notify_exec_command_begin( - &self, - sub_id: &str, - call_id: &str, - command_for_display: Vec, - command_cwd: &Path, - ) { + async fn notify_exec_command_begin(&self, exec_command_context: ExecCommandContext) { + let ExecCommandContext { + sub_id, + call_id, + command_for_display, + cwd, + apply_patch, + } = exec_command_context; + let msg = match apply_patch { + Some(ApplyPatchCommandContext { + user_explicitly_approved_this_action, + changes, + }) => EventMsg::PatchApplyBegin(PatchApplyBeginEvent { + call_id, + auto_approved: !user_explicitly_approved_this_action, + changes, + }), + None => EventMsg::ExecCommandBegin(ExecCommandBeginEvent { + call_id, + command: command_for_display.clone(), + cwd, + }), + }; let event = Event { id: sub_id.to_string(), - msg: EventMsg::ExecCommandBegin(ExecCommandBeginEvent { - call_id: call_id.to_string(), - command: command_for_display, - cwd: command_cwd.to_path_buf(), - }), + msg, }; let _ = self.tx_event.send(event).await; } @@ -383,18 +398,33 @@ impl Session { stdout: &str, stderr: &str, exit_code: i32, + is_apply_patch: bool, ) { + // Because stdout and stderr could each be up to 100 KiB, we send + // truncated versions. const MAX_STREAM_OUTPUT: usize = 5 * 1024; // 5KiB + let stdout = stdout.chars().take(MAX_STREAM_OUTPUT).collect(); + let stderr = stderr.chars().take(MAX_STREAM_OUTPUT).collect(); + + let msg = if is_apply_patch { + EventMsg::PatchApplyEnd(PatchApplyEndEvent { + call_id: call_id.to_string(), + stdout, + stderr, + success: exit_code == 0, + }) + } else { + EventMsg::ExecCommandEnd(ExecCommandEndEvent { + call_id: call_id.to_string(), + stdout, + stderr, + exit_code, + }) + }; + let event = Event { id: sub_id.to_string(), - // Because stdout and stderr could each be up to 100 KiB, we send - // truncated versions. - msg: EventMsg::ExecCommandEnd(ExecCommandEndEvent { - call_id: call_id.to_string(), - stdout: stdout.chars().take(MAX_STREAM_OUTPUT).collect(), - stderr: stderr.chars().take(MAX_STREAM_OUTPUT).collect(), - exit_code, - }), + msg, }; let _ = self.tx_event.send(event).await; } @@ -502,6 +532,21 @@ impl State { } } +#[derive(Clone, Debug)] +pub(crate) struct ExecCommandContext { + pub(crate) sub_id: String, + pub(crate) call_id: String, + pub(crate) command_for_display: Vec, + pub(crate) cwd: PathBuf, + pub(crate) apply_patch: Option, +} + +#[derive(Clone, Debug)] +pub(crate) struct ApplyPatchCommandContext { + pub(crate) user_explicitly_approved_this_action: bool, + pub(crate) changes: HashMap, +} + /// A series of Turns in response to user input. pub(crate) struct AgentTask { sess: Arc, @@ -1430,35 +1475,39 @@ async fn handle_container_exec_with_params( call_id: String, ) -> ResponseInputItem { // check if this was a patch, and apply it if so - 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), + let apply_patch_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(apply_patch_exec) => { + Some(apply_patch_exec) } } - 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, - }; + } + 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 (params, safety, command_for_display) = match apply_patch_action_for_exec { - Some(ApplyPatchAction { patch, cwd, .. }) => { + let (params, safety, command_for_display) = match &apply_patch_exec { + Some(ApplyPatchExec { + action: ApplyPatchAction { patch, cwd, .. }, + user_explicitly_approved_this_action, + }) => { let path_to_codex = std::env::current_exe() .ok() .map(|p| p.to_string_lossy().to_string()); @@ -1478,13 +1527,22 @@ async fn handle_container_exec_with_params( CODEX_APPLY_PATCH_ARG1.to_string(), patch.clone(), ], - cwd, + cwd: cwd.clone(), timeout_ms: params.timeout_ms, env: HashMap::new(), }; - let safety = - assess_safety_for_untrusted_command(sess.approval_policy, &sess.sandbox_policy); - (params, safety, vec!["apply_patch".to_string(), patch]) + let safety = if *user_explicitly_approved_this_action { + SafetyCheck::AutoApprove { + sandbox_type: SandboxType::None, + } + } else { + assess_safety_for_untrusted_command(sess.approval_policy, &sess.sandbox_policy) + }; + ( + params, + safety, + vec!["apply_patch".to_string(), patch.clone()], + ) } None => { let safety = { @@ -1545,7 +1603,22 @@ async fn handle_container_exec_with_params( } }; - sess.notify_exec_command_begin(&sub_id, &call_id, command_for_display.clone(), ¶ms.cwd) + let exec_command_context = ExecCommandContext { + sub_id: sub_id.clone(), + call_id: call_id.clone(), + command_for_display: command_for_display.clone(), + cwd: params.cwd.clone(), + apply_patch: apply_patch_exec.map( + |ApplyPatchExec { + action, + user_explicitly_approved_this_action, + }| ApplyPatchCommandContext { + user_explicitly_approved_this_action, + changes: convert_apply_patch_to_protocol(&action), + }, + ), + }; + sess.notify_exec_command_begin(exec_command_context.clone()) .await; let params = maybe_run_with_user_profile(params, sess); @@ -1567,8 +1640,15 @@ async fn handle_container_exec_with_params( duration, } = output; - sess.notify_exec_command_end(&sub_id, &call_id, &stdout, &stderr, exit_code) - .await; + sess.notify_exec_command_end( + &sub_id, + &call_id, + &stdout, + &stderr, + exit_code, + exec_command_context.apply_patch.is_some(), + ) + .await; let is_success = exit_code == 0; let content = format_exec_output( @@ -1586,16 +1666,7 @@ async fn handle_container_exec_with_params( } } Err(CodexErr::Sandbox(error)) => { - handle_sandbox_error( - error, - sandbox_type, - params, - command_for_display, - sess, - sub_id, - call_id, - ) - .await + handle_sandbox_error(params, exec_command_context, error, sandbox_type, sess).await } Err(e) => { // Handle non-sandbox errors @@ -1611,14 +1682,17 @@ async fn handle_container_exec_with_params( } async fn handle_sandbox_error( + params: ExecParams, + exec_command_context: ExecCommandContext, error: SandboxErr, sandbox_type: SandboxType, - params: ExecParams, - command_for_display: Vec, sess: &Session, - sub_id: String, - call_id: String, ) -> ResponseInputItem { + let call_id = exec_command_context.call_id.clone(); + let sub_id = exec_command_context.sub_id.clone(); + let cwd = exec_command_context.cwd.clone(); + let is_apply_patch = exec_command_context.apply_patch.is_some(); + // Early out if the user never wants to be asked for approval; just return to the model immediately if sess.approval_policy == AskForApproval::Never { return ResponseInputItem::FunctionCallOutput { @@ -1648,7 +1722,7 @@ async fn handle_sandbox_error( sub_id.clone(), call_id.clone(), params.command.clone(), - params.cwd.clone(), + cwd.clone(), Some("command failed; retry without sandbox?".to_string()), ) .await; @@ -1664,8 +1738,7 @@ async fn handle_sandbox_error( sess.notify_background_event(&sub_id, "retrying command without sandbox") .await; - sess.notify_exec_command_begin(&sub_id, &call_id, command_for_display, ¶ms.cwd) - .await; + sess.notify_exec_command_begin(exec_command_context).await; // This is an escalated retry; the policy will not be // examined and the sandbox has been set to `None`. @@ -1687,8 +1760,15 @@ async fn handle_sandbox_error( duration, } = retry_output; - sess.notify_exec_command_end(&sub_id, &call_id, &stdout, &stderr, exit_code) - .await; + sess.notify_exec_command_end( + &sub_id, + &call_id, + &stdout, + &stderr, + exit_code, + is_apply_patch, + ) + .await; let is_success = exit_code == 0; let content = format_exec_output( diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index f9bc27e058..224705f8f3 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -41,11 +41,13 @@ pub fn assess_patch_safety( } } - if is_write_patch_constrained_to_writable_paths(action, writable_roots, cwd) { - SafetyCheck::AutoApprove { - sandbox_type: SandboxType::None, - } - } else if policy == AskForApproval::OnFailure { + // Even though the patch *appears* to be constrained to writable paths, it + // is possible that paths in the patch are hard links to files outside the + // writable roots, so we should still run `apply_patch` in a sandbox in that + // case. + if is_write_patch_constrained_to_writable_paths(action, writable_roots, cwd) + || policy == AskForApproval::OnFailure + { // Only auto‑approve when we can actually enforce a sandbox. Otherwise // fall back to asking the user because the patch may touch arbitrary // paths outside the project.