From 80555d4ff25b8d7af34be207436c786de4fbaf71 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 1 Aug 2025 16:11:24 -0700 Subject: [PATCH 01/18] feat: make .git read-only within a writable root when using Seatbelt (#1765) To make `--full-auto` safer, this PR updates the Seatbelt policy so that a `SandboxPolicy` with a `writable_root` that contains a `.git/` _directory_ will make `.git/` _read-only_ (though as a follow-up, we should also consider the case where `.git` is a _file_ with a `gitdir: /path/to/actual/repo/.git` entry that should also be protected). The two major changes in this PR: - Updating `SandboxPolicy::get_writable_roots_with_cwd()` to return a `Vec` instead of a `Vec` where a `WritableRoot` can specify a list of read-only subpaths. - Updating `create_seatbelt_command_args()` to honor the read-only subpaths in `WritableRoot`. The logic to update the policy is a fairly straightforward update to `create_seatbelt_command_args()`, but perhaps the more interesting part of this PR is the introduction of an integration test in `tests/sandbox.rs`. Leveraging the new API in #1785, we test `SandboxPolicy` under various conditions, including ones where `$TMPDIR` is not readable, which is critical for verifying the new behavior. To ensure that Codex can run its own tests, e.g.: ``` just codex debug seatbelt --full-auto -- cargo test if_git_repo_is_writable_root_then_dot_git_folder_is_read_only ``` I had to introduce the use of `CODEX_SANDBOX=sandbox`, which is comparable to how `CODEX_SANDBOX_NETWORK_DISABLED=1` was already being used. Adding a comparable change for Landlock will be done in a subsequent PR. --- AGENTS.md | 4 +- codex-rs/config.md | 2 + codex-rs/core/src/protocol.rs | 56 ++++-- codex-rs/core/src/seatbelt.rs | 238 +++++++++++++++++++++++-- codex-rs/core/src/spawn.rs | 5 + codex-rs/core/tests/sandbox.rs | 195 ++++++++++++++++++++ codex-rs/linux-sandbox/src/landlock.rs | 6 +- 7 files changed, 478 insertions(+), 28 deletions(-) create mode 100644 codex-rs/core/tests/sandbox.rs diff --git a/AGENTS.md b/AGENTS.md index 27af48ae60..5c3f659c35 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,7 +2,9 @@ In the codex-rs folder where the rust code lives: -- Never add or modify any code related to `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR`. You operate in a sandbox where `CODEX_SANDBOX_NETWORK_DISABLED=1` will be set whenever you use the `shell` tool. Any existing code that uses `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` was authored with this fact in mind. It is often used to early exit out of tests that the author knew you would not be able to run given your sandbox limitations. +- Never add or modify any code related to `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` or `CODEX_SANDBOX_ENV_VAR`. + - You operate in a sandbox where `CODEX_SANDBOX_NETWORK_DISABLED=1` will be set whenever you use the `shell` tool. Any existing code that uses `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` was authored with this fact in mind. It is often used to early exit out of tests that the author knew you would not be able to run given your sandbox limitations. + - Similarly, when you spawn a process using Seatbelt (`/usr/bin/sandbox-exec`), `CODEX_SANDBOX=seatbelt` will be set on the child process. Integration tests that want to run Seatbelt themselves cannot be run under Seatbelt, so checks for `CODEX_SANDBOX=seatbelt` are also often used to early exit out of tests, as appropriate. Before creating a pull request with changes to `codex-rs`, run `just fmt` (in `codex-rs` directory) to format the code and `just fix` (in `codex-rs` directory) to fix any linter issues in the code, ensure the test suite passes by running `cargo test --all-features` in the `codex-rs` directory. diff --git a/codex-rs/config.md b/codex-rs/config.md index 1a407a239b..c7dfe42a75 100644 --- a/codex-rs/config.md +++ b/codex-rs/config.md @@ -259,6 +259,8 @@ disk, but attempts to write a file or access the network will be blocked. A more relaxed policy is `workspace-write`. When specified, the current working directory for the Codex task will be writable (as well as `$TMPDIR` on macOS). Note that the CLI defaults to using the directory where it was spawned as `cwd`, though this can be overridden using `--cwd/-C`. +On macOS (and soon Linux), all writable roots (including `cwd`) that contain a `.git/` folder _as an immediate child_ will configure the `.git/` folder to be read-only while the rest of the Git repository will be writable. This means that commands like `git commit` will fail, by default (as it entails writing to `.git/`), and will require Codex to ask for permission. + ```toml # same as `--sandbox workspace-write` sandbox_mode = "workspace-write" diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index af65f4d3a8..1bfeee56ab 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -189,6 +189,16 @@ pub enum SandboxPolicy { }, } +/// A writable root path accompanied by a list of subpaths that should remain +/// read‑only even when the root is writable. This is primarily used to ensure +/// top‑level VCS metadata directories (e.g. `.git`) under a writable root are +/// not modified by the agent. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct WritableRoot { + pub root: PathBuf, + pub read_only_subpaths: Vec, +} + fn default_true() -> bool { true } @@ -240,9 +250,10 @@ impl SandboxPolicy { } } - /// Returns the list of writable roots that should be passed down to the - /// Landlock rules installer, tailored to the current working directory. - pub fn get_writable_roots_with_cwd(&self, cwd: &Path) -> Vec { + /// Returns the list of writable roots (tailored to the current working + /// directory) together with subpaths that should remain read‑only under + /// each writable root. + pub fn get_writable_roots_with_cwd(&self, cwd: &Path) -> Vec { match self { SandboxPolicy::DangerFullAccess => Vec::new(), SandboxPolicy::ReadOnly => Vec::new(), @@ -251,24 +262,39 @@ impl SandboxPolicy { include_default_writable_roots, .. } => { - if !*include_default_writable_roots { - return writable_roots.clone(); - } + // Start from explicitly configured writable roots. + let mut roots: Vec = writable_roots.clone(); - let mut roots = writable_roots.clone(); - roots.push(cwd.to_path_buf()); + // Optionally include defaults (cwd and TMPDIR on macOS). + if *include_default_writable_roots { + roots.push(cwd.to_path_buf()); - // Also include the per-user tmp dir on macOS. - // Note this is added dynamically rather than storing it in - // writable_roots because writable_roots contains only static - // values deserialized from the config file. - if cfg!(target_os = "macos") { - if let Some(tmpdir) = std::env::var_os("TMPDIR") { - roots.push(PathBuf::from(tmpdir)); + // Also include the per-user tmp dir on macOS. + // Note this is added dynamically rather than storing it in + // `writable_roots` because `writable_roots` contains only static + // values deserialized from the config file. + if cfg!(target_os = "macos") { + if let Some(tmpdir) = std::env::var_os("TMPDIR") { + roots.push(PathBuf::from(tmpdir)); + } } } + // For each root, compute subpaths that should remain read-only. roots + .into_iter() + .map(|writable_root| { + let mut subpaths = Vec::new(); + let top_level_git = writable_root.join(".git"); + if top_level_git.is_dir() { + subpaths.push(top_level_git); + } + WritableRoot { + root: writable_root, + read_only_subpaths: subpaths, + } + }) + .collect() } } } diff --git a/codex-rs/core/src/seatbelt.rs b/codex-rs/core/src/seatbelt.rs index be2acb1bdc..0364840b1a 100644 --- a/codex-rs/core/src/seatbelt.rs +++ b/codex-rs/core/src/seatbelt.rs @@ -4,6 +4,7 @@ use std::path::PathBuf; use tokio::process::Child; use crate::protocol::SandboxPolicy; +use crate::spawn::CODEX_SANDBOX_ENV_VAR; use crate::spawn::StdioPolicy; use crate::spawn::spawn_child_async; @@ -20,10 +21,11 @@ pub async fn spawn_command_under_seatbelt( sandbox_policy: &SandboxPolicy, cwd: PathBuf, stdio_policy: StdioPolicy, - env: HashMap, + mut env: HashMap, ) -> std::io::Result { let args = create_seatbelt_command_args(command, sandbox_policy, &cwd); let arg0 = None; + env.insert(CODEX_SANDBOX_ENV_VAR.to_string(), "seatbelt".to_string()); spawn_child_async( PathBuf::from(MACOS_PATH_TO_SEATBELT_EXECUTABLE), args, @@ -50,16 +52,38 @@ fn create_seatbelt_command_args( ) } else { let writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd); - let (writable_folder_policies, cli_args): (Vec, Vec) = writable_roots - .iter() - .enumerate() - .map(|(index, root)| { - let param_name = format!("WRITABLE_ROOT_{index}"); - let policy: String = format!("(subpath (param \"{param_name}\"))"); - let cli_arg = format!("-D{param_name}={}", root.to_string_lossy()); - (policy, cli_arg) - }) - .unzip(); + + let mut writable_folder_policies: Vec = Vec::new(); + let mut cli_args: Vec = Vec::new(); + + for (index, wr) in writable_roots.iter().enumerate() { + // Canonicalize to avoid mismatches like /var vs /private/var on macOS. + let canonical_root = wr.root.canonicalize().unwrap_or_else(|_| wr.root.clone()); + let root_param = format!("WRITABLE_ROOT_{index}"); + cli_args.push(format!( + "-D{root_param}={}", + canonical_root.to_string_lossy() + )); + + if wr.read_only_subpaths.is_empty() { + writable_folder_policies.push(format!("(subpath (param \"{root_param}\"))")); + } else { + // Add parameters for each read-only subpath and generate + // the `(require-not ...)` clauses. + let mut require_parts: Vec = Vec::new(); + require_parts.push(format!("(subpath (param \"{root_param}\"))")); + for (subpath_index, ro) in wr.read_only_subpaths.iter().enumerate() { + let canonical_ro = ro.canonicalize().unwrap_or_else(|_| ro.clone()); + let ro_param = format!("WRITABLE_ROOT_{index}_RO_{subpath_index}"); + cli_args.push(format!("-D{ro_param}={}", canonical_ro.to_string_lossy())); + require_parts + .push(format!("(require-not (subpath (param \"{ro_param}\")))")); + } + let policy_component = format!("(require-all {} )", require_parts.join(" ")); + writable_folder_policies.push(policy_component); + } + } + if writable_folder_policies.is_empty() { ("".to_string(), Vec::::new()) } else { @@ -88,9 +112,201 @@ fn create_seatbelt_command_args( let full_policy = format!( "{MACOS_SEATBELT_BASE_POLICY}\n{file_read_policy}\n{file_write_policy}\n{network_policy}" ); + let mut seatbelt_args: Vec = vec!["-p".to_string(), full_policy]; seatbelt_args.extend(extra_cli_args); seatbelt_args.push("--".to_string()); seatbelt_args.extend(command); seatbelt_args } + +#[cfg(test)] +mod tests { + #![expect(clippy::expect_used)] + use super::MACOS_SEATBELT_BASE_POLICY; + use super::create_seatbelt_command_args; + use crate::protocol::SandboxPolicy; + use pretty_assertions::assert_eq; + use std::fs; + use std::path::Path; + use std::path::PathBuf; + use tempfile::TempDir; + + #[test] + fn create_seatbelt_args_with_read_only_git_subpath() { + // Create a temporary workspace with two writable roots: one containing + // a top-level .git directory and one without it. + let tmp = TempDir::new().expect("tempdir"); + let PopulatedTmp { + root_with_git, + root_without_git, + root_with_git_canon, + root_with_git_git_canon, + root_without_git_canon, + } = populate_tmpdir(tmp.path()); + + // Build a policy that only includes the two test roots as writable and + // does not automatically include defaults like cwd or TMPDIR. + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![root_with_git.clone(), root_without_git.clone()], + network_access: false, + include_default_writable_roots: false, + }; + + let args = create_seatbelt_command_args( + vec!["/bin/echo".to_string(), "hello".to_string()], + &policy, + tmp.path(), + ); + + // Build the expected policy text using a raw string for readability. + // Note that the policy includes: + // - the base policy, + // - read-only access to the filesystem, + // - write access to WRITABLE_ROOT_0 (but not its .git) and WRITABLE_ROOT_1. + let expected_policy = format!( + r#"{MACOS_SEATBELT_BASE_POLICY} +; allow read-only file operations +(allow file-read*) +(allow file-write* +(require-all (subpath (param "WRITABLE_ROOT_0")) (require-not (subpath (param "WRITABLE_ROOT_0_RO_0"))) ) (subpath (param "WRITABLE_ROOT_1")) +) +"#, + ); + + let expected_args = vec![ + "-p".to_string(), + expected_policy, + format!( + "-DWRITABLE_ROOT_0={}", + root_with_git_canon.to_string_lossy() + ), + format!( + "-DWRITABLE_ROOT_0_RO_0={}", + root_with_git_git_canon.to_string_lossy() + ), + format!( + "-DWRITABLE_ROOT_1={}", + root_without_git_canon.to_string_lossy() + ), + "--".to_string(), + "/bin/echo".to_string(), + "hello".to_string(), + ]; + + assert_eq!(args, expected_args); + } + + #[test] + fn create_seatbelt_args_for_cwd_as_git_repo() { + // Create a temporary workspace with two writable roots: one containing + // a top-level .git directory and one without it. + let tmp = TempDir::new().expect("tempdir"); + let PopulatedTmp { + root_with_git, + root_with_git_canon, + root_with_git_git_canon, + .. + } = populate_tmpdir(tmp.path()); + + // Build a policy that does not specify any writable_roots, but does + // use the default ones (cwd and TMPDIR) and verifies the `.git` check + // is done properly for cwd. + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + include_default_writable_roots: true, + }; + + let args = create_seatbelt_command_args( + vec!["/bin/echo".to_string(), "hello".to_string()], + &policy, + root_with_git.as_path(), + ); + + let tmpdir_env_var = if cfg!(target_os = "macos") { + std::env::var("TMPDIR") + .ok() + .map(PathBuf::from) + .and_then(|p| p.canonicalize().ok()) + .map(|p| p.to_string_lossy().to_string()) + } else { + None + }; + let tempdir_policy_entry = if tmpdir_env_var.is_some() { + " (subpath (param \"WRITABLE_ROOT_1\"))" + } else { + "" + }; + + // Build the expected policy text using a raw string for readability. + // Note that the policy includes: + // - the base policy, + // - read-only access to the filesystem, + // - write access to WRITABLE_ROOT_0 (but not its .git) and WRITABLE_ROOT_1. + let expected_policy = format!( + r#"{MACOS_SEATBELT_BASE_POLICY} +; allow read-only file operations +(allow file-read*) +(allow file-write* +(require-all (subpath (param "WRITABLE_ROOT_0")) (require-not (subpath (param "WRITABLE_ROOT_0_RO_0"))) ){tempdir_policy_entry} +) +"#, + ); + + let mut expected_args = vec![ + "-p".to_string(), + expected_policy, + format!( + "-DWRITABLE_ROOT_0={}", + root_with_git_canon.to_string_lossy() + ), + format!( + "-DWRITABLE_ROOT_0_RO_0={}", + root_with_git_git_canon.to_string_lossy() + ), + ]; + + if let Some(p) = tmpdir_env_var { + expected_args.push(format!("-DWRITABLE_ROOT_1={p}")); + } + + expected_args.extend(vec![ + "--".to_string(), + "/bin/echo".to_string(), + "hello".to_string(), + ]); + + assert_eq!(args, expected_args); + } + + struct PopulatedTmp { + root_with_git: PathBuf, + root_without_git: PathBuf, + root_with_git_canon: PathBuf, + root_with_git_git_canon: PathBuf, + root_without_git_canon: PathBuf, + } + + fn populate_tmpdir(tmp: &Path) -> PopulatedTmp { + let root_with_git = tmp.join("with_git"); + let root_without_git = tmp.join("no_git"); + fs::create_dir_all(&root_with_git).expect("create with_git"); + fs::create_dir_all(&root_without_git).expect("create no_git"); + fs::create_dir_all(root_with_git.join(".git")).expect("create .git"); + + // Ensure we have canonical paths for -D parameter matching. + let root_with_git_canon = root_with_git.canonicalize().expect("canonicalize with_git"); + let root_with_git_git_canon = root_with_git_canon.join(".git"); + let root_without_git_canon = root_without_git + .canonicalize() + .expect("canonicalize no_git"); + PopulatedTmp { + root_with_git, + root_without_git, + root_with_git_canon, + root_with_git_git_canon, + root_without_git_canon, + } + } +} diff --git a/codex-rs/core/src/spawn.rs b/codex-rs/core/src/spawn.rs index 9fde26539b..1c82df3180 100644 --- a/codex-rs/core/src/spawn.rs +++ b/codex-rs/core/src/spawn.rs @@ -17,6 +17,11 @@ use crate::protocol::SandboxPolicy; /// attributes, so this may change in the future. pub const CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR: &str = "CODEX_SANDBOX_NETWORK_DISABLED"; +/// Should be set when the process is spawned under a sandbox. Currently, the +/// value is "seatbelt" for macOS, but it may change in the future to +/// accommodate sandboxing configuration and other sandboxing mechanisms. +pub const CODEX_SANDBOX_ENV_VAR: &str = "CODEX_SANDBOX"; + #[derive(Debug, Clone, Copy)] pub enum StdioPolicy { RedirectForShellTool, diff --git a/codex-rs/core/tests/sandbox.rs b/codex-rs/core/tests/sandbox.rs new file mode 100644 index 0000000000..e85156bf05 --- /dev/null +++ b/codex-rs/core/tests/sandbox.rs @@ -0,0 +1,195 @@ +#![cfg(target_os = "macos")] +#![expect(clippy::expect_used)] + +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; + +use codex_core::protocol::SandboxPolicy; +use codex_core::seatbelt::spawn_command_under_seatbelt; +use codex_core::spawn::CODEX_SANDBOX_ENV_VAR; +use codex_core::spawn::StdioPolicy; +use tempfile::TempDir; + +struct TestScenario { + repo_parent: PathBuf, + file_outside_repo: PathBuf, + repo_root: PathBuf, + file_in_repo_root: PathBuf, + file_in_dot_git_dir: PathBuf, +} + +struct TestExpectations { + file_outside_repo_is_writable: bool, + file_in_repo_root_is_writable: bool, + file_in_dot_git_dir_is_writable: bool, +} + +impl TestScenario { + async fn run_test(&self, policy: &SandboxPolicy, expectations: TestExpectations) { + if std::env::var(CODEX_SANDBOX_ENV_VAR) == Ok("seatbelt".to_string()) { + eprintln!("{CODEX_SANDBOX_ENV_VAR} is set to 'seatbelt', skipping test."); + return; + } + + assert_eq!( + touch(&self.file_outside_repo, policy).await, + expectations.file_outside_repo_is_writable + ); + assert_eq!( + self.file_outside_repo.exists(), + expectations.file_outside_repo_is_writable + ); + + assert_eq!( + touch(&self.file_in_repo_root, policy).await, + expectations.file_in_repo_root_is_writable + ); + assert_eq!( + self.file_in_repo_root.exists(), + expectations.file_in_repo_root_is_writable + ); + + assert_eq!( + touch(&self.file_in_dot_git_dir, policy).await, + expectations.file_in_dot_git_dir_is_writable + ); + assert_eq!( + self.file_in_dot_git_dir.exists(), + expectations.file_in_dot_git_dir_is_writable + ); + } +} + +/// If the user has added a workspace root that is not a Git repo root, then +/// the user has to specify `--skip-git-repo-check` or go through some +/// interstitial that indicates they are taking on some risk because Git +/// cannot be used to backup their work before the agent begins. +/// +/// Because the user has agreed to this risk, we do not try find all .git +/// folders in the workspace and block them (though we could change our +/// position on this in the future). +#[tokio::test] +async fn if_parent_of_repo_is_writable_then_dot_git_folder_is_writable() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let test_scenario = create_test_scenario(&tmp); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![test_scenario.repo_parent.clone()], + network_access: false, + include_default_writable_roots: false, + }; + + test_scenario + .run_test( + &policy, + TestExpectations { + file_outside_repo_is_writable: true, + file_in_repo_root_is_writable: true, + file_in_dot_git_dir_is_writable: true, + }, + ) + .await; +} + +/// When the writable root is the root of a Git repository (as evidenced by the +/// presence of a .git folder), then the .git folder should be read-only if +/// the policy is `WorkspaceWrite`. +#[tokio::test] +async fn if_git_repo_is_writable_root_then_dot_git_folder_is_read_only() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let test_scenario = create_test_scenario(&tmp); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![test_scenario.repo_root.clone()], + network_access: false, + include_default_writable_roots: false, + }; + + test_scenario + .run_test( + &policy, + TestExpectations { + file_outside_repo_is_writable: false, + file_in_repo_root_is_writable: true, + file_in_dot_git_dir_is_writable: false, + }, + ) + .await; +} + +/// Under DangerFullAccess, all writes should be permitted anywhere on disk, +/// including inside the .git folder. +#[tokio::test] +async fn danger_full_access_allows_all_writes() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let test_scenario = create_test_scenario(&tmp); + let policy = SandboxPolicy::DangerFullAccess; + + test_scenario + .run_test( + &policy, + TestExpectations { + file_outside_repo_is_writable: true, + file_in_repo_root_is_writable: true, + file_in_dot_git_dir_is_writable: true, + }, + ) + .await; +} + +/// Under ReadOnly, writes should not be permitted anywhere on disk. +#[tokio::test] +async fn read_only_forbids_all_writes() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let test_scenario = create_test_scenario(&tmp); + let policy = SandboxPolicy::ReadOnly; + + test_scenario + .run_test( + &policy, + TestExpectations { + file_outside_repo_is_writable: false, + file_in_repo_root_is_writable: false, + file_in_dot_git_dir_is_writable: false, + }, + ) + .await; +} + +fn create_test_scenario(tmp: &TempDir) -> TestScenario { + let repo_parent = tmp.path().to_path_buf(); + let repo_root = repo_parent.join("repo"); + let dot_git_dir = repo_root.join(".git"); + + std::fs::create_dir(&repo_root).expect("should be able to create repo root"); + std::fs::create_dir(&dot_git_dir).expect("should be able to create .git dir"); + + TestScenario { + file_outside_repo: repo_parent.join("outside.txt"), + repo_parent, + file_in_repo_root: repo_root.join("repo_file.txt"), + repo_root, + file_in_dot_git_dir: dot_git_dir.join("dot_git_file.txt"), + } +} + +/// Note that `path` must be absolute. +async fn touch(path: &Path, policy: &SandboxPolicy) -> bool { + assert!(path.is_absolute(), "Path must be absolute: {path:?}"); + let mut child = spawn_command_under_seatbelt( + vec![ + "/usr/bin/touch".to_string(), + path.to_string_lossy().to_string(), + ], + policy, + std::env::current_dir().expect("should be able to get current dir"), + StdioPolicy::RedirectForShellTool, + HashMap::new(), + ) + .await + .expect("should be able to spawn command under seatbelt"); + child + .wait() + .await + .expect("should be able to wait for child process") + .success() +} diff --git a/codex-rs/linux-sandbox/src/landlock.rs b/codex-rs/linux-sandbox/src/landlock.rs index 326e2cb487..e13e3c8b75 100644 --- a/codex-rs/linux-sandbox/src/landlock.rs +++ b/codex-rs/linux-sandbox/src/landlock.rs @@ -36,7 +36,11 @@ pub(crate) fn apply_sandbox_policy_to_current_thread( } if !sandbox_policy.has_full_disk_write_access() { - let writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd); + let writable_roots = sandbox_policy + .get_writable_roots_with_cwd(cwd) + .into_iter() + .map(|writable_root| writable_root.root) + .collect(); install_filesystem_landlock_rules_on_current_thread(writable_roots)?; } From 929ba50adce36c3b3e5cf0af42e38347a57bb09b Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 1 Aug 2025 16:30:15 -0700 Subject: [PATCH 02/18] Update succesfull login page look (#1789) --- codex-rs/login/src/login_with_chatgpt.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/codex-rs/login/src/login_with_chatgpt.py b/codex-rs/login/src/login_with_chatgpt.py index 2dbf5be58a..4c07feeba0 100644 --- a/codex-rs/login/src/login_with_chatgpt.py +++ b/codex-rs/login/src/login_with_chatgpt.py @@ -686,6 +686,7 @@ LOGIN_SUCCESS_HTML = """ justify-content: center; position: relative; background: white; + font-family: system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, Ubuntu, Cantarell, 'Open Sans', 'Helvetica Neue', sans-serif; } .inner-container { @@ -703,6 +704,7 @@ LOGIN_SUCCESS_HTML = """ align-items: center; gap: 20px; display: flex; + margin-top: 15vh; } .svg-wrapper { position: relative; @@ -710,9 +712,9 @@ LOGIN_SUCCESS_HTML = """ .title { text-align: center; color: var(--text-primary, #0D0D0D); - font-size: 28px; + font-size: 32px; font-weight: 400; - line-height: 36.40px; + line-height: 40px; word-wrap: break-word; } .setup-box { @@ -785,16 +787,26 @@ LOGIN_SUCCESS_HTML = """ word-wrap: break-word; text-decoration: none; } + .logo { + display: flex; + align-items: center; + justify-content: center; + width: 4rem; + height: 4rem; + border-radius: 16px; + border: .5px solid rgba(0, 0, 0, 0.1); + box-shadow: rgba(0, 0, 0, 0.1) 0px 4px 16px 0px; + box-sizing: border-box; + background-color: rgb(255, 255, 255); + }
-
- - - +
Signed in to Codex CLI
From 7e0f506da208d6ee9c289cc28b13532226d5abdb Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Fri, 1 Aug 2025 17:31:38 -0700 Subject: [PATCH 03/18] check for updates (#1764) 1. Ping https://api.github.com/repos/openai/codex/releases/latest (at most once every 20 hrs) 2. Store the result in ~/.codex/version.jsonl 3. If CARGO_PKG_VERSION < latest_version, print a message at boot. --------- Co-authored-by: easong-openai --- codex-cli/bin/codex.js | 1 + codex-rs/Cargo.lock | 3 + codex-rs/tui/Cargo.toml | 3 + codex-rs/tui/src/lib.rs | 37 ++++++++++ codex-rs/tui/src/updates.rs | 137 ++++++++++++++++++++++++++++++++++++ 5 files changed, 181 insertions(+) create mode 100644 codex-rs/tui/src/updates.rs diff --git a/codex-cli/bin/codex.js b/codex-cli/bin/codex.js index ae1fb9593c..df06dd36a7 100755 --- a/codex-cli/bin/codex.js +++ b/codex-cli/bin/codex.js @@ -83,6 +83,7 @@ if (wantsNative && process.platform !== 'win32') { const child = spawn(binaryPath, process.argv.slice(2), { stdio: "inherit", + env: { ...process.env, CODEX_MANAGED_BY_NPM: "1" }, }); child.on("error", (err) => { diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 460a440e63..d71553cf4a 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -843,6 +843,7 @@ version = "0.0.0" dependencies = [ "anyhow", "base64 0.22.1", + "chrono", "clap", "codex-ansi-escape", "codex-arg0", @@ -861,6 +862,8 @@ dependencies = [ "ratatui", "ratatui-image", "regex-lite", + "reqwest", + "serde", "serde_json", "shlex", "strum 0.27.2", diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 468f2f3be6..09b537c6c3 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -17,6 +17,7 @@ workspace = true [dependencies] anyhow = "1" base64 = "0.22.1" +chrono = { version = "0.4", features = ["serde"] } clap = { version = "4", features = ["derive"] } codex-ansi-escape = { path = "../ansi-escape" } codex-arg0 = { path = "../arg0" } @@ -41,6 +42,8 @@ ratatui = { version = "0.29.0", features = [ ] } ratatui-image = "8.0.0" regex-lite = "0.1" +reqwest = { version = "0.12", features = ["json"] } +serde = { version = "1", features = ["derive"] } serde_json = { version = "1", features = ["preserve_order"] } shlex = "1.3.0" strum = "0.27.2" diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index f0a0e9d833..0ec9be6153 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -41,6 +41,11 @@ mod text_formatting; mod tui; mod user_approval_widget; +#[cfg(not(debug_assertions))] +mod updates; +#[cfg(not(debug_assertions))] +use color_eyre::owo_colors::OwoColorize; + pub use cli::Cli; pub async fn run_main( @@ -139,6 +144,38 @@ pub async fn run_main( .with(tui_layer) .try_init(); + #[allow(clippy::print_stderr)] + #[cfg(not(debug_assertions))] + if let Some(latest_version) = updates::get_upgrade_version(&config) { + let current_version = env!("CARGO_PKG_VERSION"); + let exe = std::env::current_exe()?; + let managed_by_npm = std::env::var_os("CODEX_MANAGED_BY_NPM").is_some(); + + eprintln!( + "{} {current_version} -> {latest_version}.", + "✨⬆️ Update available!".bold().cyan() + ); + + if managed_by_npm { + let npm_cmd = "npm install -g @openai/codex@latest"; + eprintln!("Run {} to update.", npm_cmd.cyan().on_black()); + } else if cfg!(target_os = "macos") + && (exe.starts_with("/opt/homebrew") || exe.starts_with("/usr/local")) + { + let brew_cmd = "brew upgrade codex"; + eprintln!("Run {} to update.", brew_cmd.cyan().on_black()); + } else { + eprintln!( + "See {} for the latest releases and installation options.", + "https://github.com/openai/codex/releases/latest" + .cyan() + .on_black() + ); + } + + eprintln!(""); + } + let show_login_screen = should_show_login_screen(&config); if show_login_screen { std::io::stdout() diff --git a/codex-rs/tui/src/updates.rs b/codex-rs/tui/src/updates.rs new file mode 100644 index 0000000000..c7f7afd2a5 --- /dev/null +++ b/codex-rs/tui/src/updates.rs @@ -0,0 +1,137 @@ +#![cfg(any(not(debug_assertions), test))] + +use chrono::DateTime; +use chrono::Duration; +use chrono::Utc; +use serde::Deserialize; +use serde::Serialize; +use std::path::Path; +use std::path::PathBuf; + +use codex_core::config::Config; + +pub fn get_upgrade_version(config: &Config) -> Option { + let version_file = version_filepath(config); + let info = read_version_info(&version_file).ok(); + + if match &info { + None => true, + Some(info) => info.last_checked_at < Utc::now() - Duration::hours(20), + } { + // Refresh the cached latest version in the background so TUI startup + // isn’t blocked by a network call. The UI reads the previously cached + // value (if any) for this run; the next run shows the banner if needed. + tokio::spawn(async move { + check_for_update(&version_file) + .await + .inspect_err(|e| tracing::error!("Failed to update version: {e}")) + }); + } + + info.and_then(|info| { + let current_version = env!("CARGO_PKG_VERSION"); + if is_newer(&info.latest_version, current_version).unwrap_or(false) { + Some(info.latest_version) + } else { + None + } + }) +} + +#[derive(Serialize, Deserialize, Debug, Clone)] +struct VersionInfo { + latest_version: String, + // ISO-8601 timestamp (RFC3339) + last_checked_at: DateTime, +} + +#[derive(Deserialize, Debug, Clone)] +struct ReleaseInfo { + tag_name: String, +} + +const VERSION_FILENAME: &str = "version.json"; +const LATEST_RELEASE_URL: &str = "https://api.github.com/repos/openai/codex/releases/latest"; + +fn version_filepath(config: &Config) -> PathBuf { + config.codex_home.join(VERSION_FILENAME) +} + +fn read_version_info(version_file: &Path) -> anyhow::Result { + let contents = std::fs::read_to_string(version_file)?; + Ok(serde_json::from_str(&contents)?) +} + +async fn check_for_update(version_file: &Path) -> anyhow::Result<()> { + let ReleaseInfo { + tag_name: latest_tag_name, + } = reqwest::Client::new() + .get(LATEST_RELEASE_URL) + .header( + "User-Agent", + format!( + "codex/{} (+https://github.com/openai/codex)", + env!("CARGO_PKG_VERSION") + ), + ) + .send() + .await? + .error_for_status()? + .json::() + .await?; + + let info = VersionInfo { + latest_version: latest_tag_name + .strip_prefix("rust-v") + .ok_or_else(|| anyhow::anyhow!("Failed to parse latest tag name '{latest_tag_name}'"))? + .into(), + last_checked_at: Utc::now(), + }; + + let json_line = format!("{}\n", serde_json::to_string(&info)?); + if let Some(parent) = version_file.parent() { + tokio::fs::create_dir_all(parent).await?; + } + tokio::fs::write(version_file, json_line).await?; + Ok(()) +} + +fn is_newer(latest: &str, current: &str) -> Option { + match (parse_version(latest), parse_version(current)) { + (Some(l), Some(c)) => Some(l > c), + _ => None, + } +} + +fn parse_version(v: &str) -> Option<(u64, u64, u64)> { + let mut iter = v.trim().split('.'); + let maj = iter.next()?.parse::().ok()?; + let min = iter.next()?.parse::().ok()?; + let pat = iter.next()?.parse::().ok()?; + Some((maj, min, pat)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn prerelease_version_is_not_considered_newer() { + assert_eq!(is_newer("0.11.0-beta.1", "0.11.0"), None); + assert_eq!(is_newer("1.0.0-rc.1", "1.0.0"), None); + } + + #[test] + fn plain_semver_comparisons_work() { + assert_eq!(is_newer("0.11.1", "0.11.0"), Some(true)); + assert_eq!(is_newer("0.11.0", "0.11.1"), Some(false)); + assert_eq!(is_newer("1.0.0", "0.9.9"), Some(true)); + assert_eq!(is_newer("0.9.9", "1.0.0"), Some(false)); + } + + #[test] + fn whitespace_is_ignored() { + assert_eq!(parse_version(" 1.2.3 \n"), Some((1, 2, 3))); + assert_eq!(is_newer(" 1.2.3 ", "1.2.2"), Some(true)); + } +} From 81bb1c9e264095708a01f6326bf8d527a6b2d47b Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Sat, 2 Aug 2025 12:05:06 -0700 Subject: [PATCH 04/18] Fix compact (#1798) We are not recording the summary in the history. --- codex-rs/core/src/codex.rs | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 6e21642da1..e759acb4a2 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1373,7 +1373,7 @@ async fn run_compact_task( let mut retries = 0; loop { - let attempt_result = drain_to_completed(&sess, &prompt).await; + let attempt_result = drain_to_completed(&sess, &sub_id, &prompt).await; match attempt_result { Ok(()) => break, @@ -2001,7 +2001,7 @@ fn get_last_assistant_message_from_turn(responses: &[ResponseItem]) -> Option CodexResult<()> { +async fn drain_to_completed(sess: &Session, sub_id: &str, prompt: &Prompt) -> CodexResult<()> { let mut stream = sess.client.clone().stream(prompt).await?; loop { let maybe_event = stream.next().await; @@ -2011,7 +2011,32 @@ async fn drain_to_completed(sess: &Session, prompt: &Prompt) -> CodexResult<()> )); }; match event { - Ok(ResponseEvent::Completed { .. }) => return Ok(()), + Ok(ResponseEvent::OutputItemDone(item)) => { + // Record only to in-memory conversation history; avoid state snapshot. + let mut state = sess.state.lock().unwrap(); + state.history.record_items(std::slice::from_ref(&item)); + } + Ok(ResponseEvent::Completed { + response_id: _, + token_usage, + }) => { + let token_usage = match token_usage { + Some(usage) => usage, + None => { + return Err(CodexErr::Stream( + "token_usage was None in ResponseEvent::Completed".into(), + )); + } + }; + sess.tx_event + .send(Event { + id: sub_id.to_string(), + msg: EventMsg::TokenCount(token_usage), + }) + .await + .ok(); + return Ok(()); + } Ok(_) => continue, Err(e) => return Err(e), } From 75eecb656e227f5b02924cabd4a98ed798f294d8 Mon Sep 17 00:00:00 2001 From: David Z Hao Date: Sun, 3 Aug 2025 06:59:26 -0700 Subject: [PATCH 05/18] Fix MacOS multiprocessing by relaxing sandbox (#1808) The following test script fails in the codex sandbox: ``` import multiprocessing from multiprocessing import Lock, Process def f(lock): with lock: print("Lock acquired in child process") if __name__ == '__main__': lock = Lock() p = Process(target=f, args=(lock,)) p.start() p.join() ``` with ``` Traceback (most recent call last): File "/Users/david.hao/code/codex/codex-rs/cli/test.py", line 9, in lock = Lock() ^^^^^^ File "/Users/david.hao/.local/share/uv/python/cpython-3.12.9-macos-aarch64-none/lib/python3.12/multiprocessing/context.py", line 68, in Lock return Lock(ctx=self.get_context()) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/david.hao/.local/share/uv/python/cpython-3.12.9-macos-aarch64-none/lib/python3.12/multiprocessing/synchronize.py", line 169, in __init__ SemLock.__init__(self, SEMAPHORE, 1, 1, ctx=ctx) File "/Users/david.hao/.local/share/uv/python/cpython-3.12.9-macos-aarch64-none/lib/python3.12/multiprocessing/synchronize.py", line 57, in __init__ sl = self._semlock = _multiprocessing.SemLock( ^^^^^^^^^^^^^^^^^^^^^^^^^ PermissionError: [Errno 1] Operation not permitted ``` After reading, adding this line to the sandbox configs fixes things - MacOS multiprocessing appears to use sem_lock(), which opens an IPC which is considered a disk write even though no file is created. I interrogated ChatGPT about whether it's okay to loosen, and my impression after reading is that it is, although would appreciate a close look Breadcrumb: You can run `cargo run -- debug seatbelt --full-auto ` to test the sandbox --- codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts | 6 +++++- codex-rs/core/src/seatbelt_base_policy.sbpl | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts b/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts index 290e6b5630..162fe908c4 100644 --- a/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts +++ b/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts @@ -147,4 +147,8 @@ const READ_ONLY_SEATBELT_POLICY = ` (sysctl-name "kern.version") (sysctl-name "sysctl.proc_cputype") (sysctl-name-prefix "hw.perflevel") -)`.trim(); +) + +; Added on top of Chrome profile +; Needed for python multiprocessing on MacOS for the SemLock +(allow ipc-posix-sem)`.trim(); diff --git a/codex-rs/core/src/seatbelt_base_policy.sbpl b/codex-rs/core/src/seatbelt_base_policy.sbpl index c9664651c2..b250494836 100644 --- a/codex-rs/core/src/seatbelt_base_policy.sbpl +++ b/codex-rs/core/src/seatbelt_base_policy.sbpl @@ -65,3 +65,7 @@ (sysctl-name "sysctl.proc_cputype") (sysctl-name-prefix "hw.perflevel") ) + +; Added on top of Chrome profile +; Needed for python multiprocessing on MacOS for the SemLock +(allow ipc-posix-sem) From 4c9f7b6bcc4a135162e792b9adb2b245f0c4976d Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Sun, 3 Aug 2025 07:19:12 -0700 Subject: [PATCH 06/18] Fix flaky test_shell_command_approval_triggers_elicitation test (#1802) This doesn't flake very often but this should fix it. --- codex-rs/mcp-server/src/exec_approval.rs | 2 +- codex-rs/mcp-server/tests/codex_tool.rs | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/codex-rs/mcp-server/src/exec_approval.rs b/codex-rs/mcp-server/src/exec_approval.rs index f073214bf5..54abfb35bd 100644 --- a/codex-rs/mcp-server/src/exec_approval.rs +++ b/codex-rs/mcp-server/src/exec_approval.rs @@ -18,7 +18,7 @@ use crate::codex_tool_runner::INVALID_PARAMS_ERROR_CODE; /// Conforms to [`mcp_types::ElicitRequestParams`] so that it can be used as the /// `params` field of an [`ElicitRequest`]. -#[derive(Debug, Serialize)] +#[derive(Debug, Deserialize, Serialize)] pub struct ExecApprovalElicitRequestParams { // These fields are required so that `params` // conforms to ElicitRequestParams. diff --git a/codex-rs/mcp-server/tests/codex_tool.rs b/codex-rs/mcp-server/tests/codex_tool.rs index 7d59ff202e..fc992a8cd1 100644 --- a/codex-rs/mcp-server/tests/codex_tool.rs +++ b/codex-rs/mcp-server/tests/codex_tool.rs @@ -89,14 +89,18 @@ async fn shell_command_approval_triggers_elicitation() -> anyhow::Result<()> { // This is the first request from the server, so the id should be 0 given // how things are currently implemented. let elicitation_request_id = RequestId::Integer(0); + let params = serde_json::from_value::( + elicitation_request + .params + .clone() + .ok_or_else(|| anyhow::anyhow!("elicitation_request.params must be set"))?, + )?; let expected_elicitation_request = create_expected_elicitation_request( elicitation_request_id.clone(), shell_command.clone(), workdir_for_shell_function_call.path(), codex_request_id.to_string(), - // Internal Codex id: empirically it is 1, but this is - // admittedly an internal detail that could change. - "1".to_string(), + params.codex_event_id.clone(), )?; assert_eq!(expected_elicitation_request, elicitation_request); From d62b703a211f6e06fc81b3f96671f431e1e59252 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Sun, 3 Aug 2025 11:31:35 -0700 Subject: [PATCH 07/18] custom textarea (#1794) This replaces tui-textarea with a custom textarea component. Key differences: 1. wrapped lines 2. better unicode handling 3. uses the native terminal cursor This should perhaps be spun out into its own separate crate at some point, but for now it's convenient to have it in-tree. --- codex-rs/Cargo.lock | 39 +- codex-rs/tui/Cargo.toml | 4 +- codex-rs/tui/src/app.rs | 13 +- codex-rs/tui/src/bottom_pane/chat_composer.rs | 637 ++++---- .../src/bottom_pane/chat_composer_history.rs | 91 +- codex-rs/tui/src/bottom_pane/mod.rs | 17 +- codex-rs/tui/src/bottom_pane/textarea.rs | 1294 +++++++++++++++++ codex-rs/tui/src/chatwidget.rs | 4 + 8 files changed, 1690 insertions(+), 409 deletions(-) create mode 100644 codex-rs/tui/src/bottom_pane/textarea.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index d71553cf4a..e1a0e162dc 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -859,6 +859,7 @@ dependencies = [ "mcp-types", "path-clean", "pretty_assertions", + "rand 0.8.5", "ratatui", "ratatui-image", "regex-lite", @@ -868,13 +869,13 @@ dependencies = [ "shlex", "strum 0.27.2", "strum_macros 0.27.2", + "textwrap 0.16.2", "tokio", "tracing", "tracing-appender", "tracing-subscriber", "tui-input", "tui-markdown", - "tui-textarea", "unicode-segmentation", "unicode-width 0.1.14", "uuid", @@ -4173,6 +4174,12 @@ version = "1.15.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "67b1b7a3b5fe4f1376887184045fcf45c69e92af734b7aaddc05fb777b6fbd03" +[[package]] +name = "smawk" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b7c388c1b5e93756d0c740965c41e8822f866621d41acbdf6336a6a168f8840c" + [[package]] name = "socket2" version = "0.5.10" @@ -4235,7 +4242,7 @@ dependencies = [ "starlark_syntax", "static_assertions", "strsim 0.10.0", - "textwrap", + "textwrap 0.11.0", "thiserror 1.0.69", ] @@ -4524,6 +4531,17 @@ dependencies = [ "unicode-width 0.1.14", ] +[[package]] +name = "textwrap" +version = "0.16.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c13547615a44dc9c452a8a534638acdf07120d4b6847c8178705da06306a3057" +dependencies = [ + "smawk", + "unicode-linebreak", + "unicode-width 0.2.0", +] + [[package]] name = "thiserror" version = "1.0.69" @@ -4988,17 +5006,6 @@ dependencies = [ "tracing", ] -[[package]] -name = "tui-textarea" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0a5318dd619ed73c52a9417ad19046724effc1287fb75cdcc4eca1d6ac1acbae" -dependencies = [ - "crossterm", - "ratatui", - "unicode-width 0.2.0", -] - [[package]] name = "typenum" version = "1.18.0" @@ -5017,6 +5024,12 @@ version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5a5f39404a5da50712a4c1eecf25e90dd62b613502b7e925fd4e4d19b5c96512" +[[package]] +name = "unicode-linebreak" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3b09c83c3c29d37506a3e260c08c03743a6bb66a9cd432c6934ab501a190571f" + [[package]] name = "unicode-segmentation" version = "1.12.0" diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 09b537c6c3..823fd1428e 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -48,6 +48,7 @@ serde_json = { version = "1", features = ["preserve_order"] } shlex = "1.3.0" strum = "0.27.2" strum_macros = "0.27.2" +textwrap = "0.16.2" tokio = { version = "1", features = [ "io-std", "macros", @@ -60,7 +61,6 @@ tracing-appender = "0.2.3" tracing-subscriber = { version = "0.3.19", features = ["env-filter"] } tui-input = "0.14.0" tui-markdown = "0.3.3" -tui-textarea = "0.7.0" unicode-segmentation = "1.12.0" unicode-width = "0.1" uuid = "1" @@ -70,3 +70,5 @@ uuid = "1" [dev-dependencies] insta = "1.43.1" pretty_assertions = "1" +rand = "0.8" +chrono = { version = "0.4", features = ["serde"] } diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index efaec7f19a..1142bd87fc 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -438,14 +438,15 @@ impl App<'_> { ); self.pending_history_lines.clear(); } - match &mut self.app_state { + terminal.draw(|frame| match &mut self.app_state { AppState::Chat { widget } => { - terminal.draw(|frame| frame.render_widget_ref(&**widget, frame.area()))?; + if let Some((x, y)) = widget.cursor_pos(frame.area()) { + frame.set_cursor_position((x, y)); + } + frame.render_widget_ref(&**widget, frame.area()) } - AppState::GitWarning { screen } => { - terminal.draw(|frame| frame.render_widget_ref(&*screen, frame.area()))?; - } - } + AppState::GitWarning { screen } => frame.render_widget_ref(&*screen, frame.area()), + })?; Ok(()) } diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 7e187bec8b..c9ad719771 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -1,6 +1,11 @@ use codex_core::protocol::TokenUsage; +use crossterm::event::KeyCode; use crossterm::event::KeyEvent; +use crossterm::event::KeyModifiers; use ratatui::buffer::Buffer; +use ratatui::layout::Constraint; +use ratatui::layout::Layout; +use ratatui::layout::Margin; use ratatui::layout::Rect; use ratatui::style::Color; use ratatui::style::Style; @@ -8,13 +13,11 @@ use ratatui::style::Styled; use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; +use ratatui::widgets::Block; use ratatui::widgets::BorderType; use ratatui::widgets::Borders; -use ratatui::widgets::Widget; +use ratatui::widgets::StatefulWidgetRef; use ratatui::widgets::WidgetRef; -use tui_textarea::Input; -use tui_textarea::Key; -use tui_textarea::TextArea; use super::chat_composer_history::ChatComposerHistory; use super::command_popup::CommandPopup; @@ -22,7 +25,10 @@ use super::file_search_popup::FileSearchPopup; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; +use crate::bottom_pane::textarea::TextArea; +use crate::bottom_pane::textarea::TextAreaState; use codex_file_search::FileMatch; +use std::cell::RefCell; const BASE_PLACEHOLDER_TEXT: &str = "..."; /// If the pasted content exceeds this number of characters, replace it with a @@ -35,8 +41,14 @@ pub enum InputResult { None, } -pub(crate) struct ChatComposer<'a> { - textarea: TextArea<'a>, +struct TokenUsageInfo { + token_usage: TokenUsage, + model_context_window: Option, +} + +pub(crate) struct ChatComposer { + textarea: TextArea, + textarea_state: RefCell, active_popup: ActivePopup, app_event_tx: AppEventSender, history: ChatComposerHistory, @@ -45,6 +57,8 @@ pub(crate) struct ChatComposer<'a> { dismissed_file_popup_token: Option, current_file_query: Option, pending_pastes: Vec<(String, String)>, + token_usage_info: Option, + has_focus: bool, } /// Popup state – at most one can be visible at any time. @@ -54,20 +68,17 @@ enum ActivePopup { File(FileSearchPopup), } -impl ChatComposer<'_> { +impl ChatComposer { pub fn new( has_input_focus: bool, app_event_tx: AppEventSender, enhanced_keys_supported: bool, ) -> Self { - let mut textarea = TextArea::default(); - textarea.set_placeholder_text(BASE_PLACEHOLDER_TEXT); - textarea.set_cursor_line_style(ratatui::style::Style::default()); - let use_shift_enter_hint = enhanced_keys_supported; - let mut this = Self { - textarea, + Self { + textarea: TextArea::new(), + textarea_state: RefCell::new(TextAreaState::default()), active_popup: ActivePopup::None, app_event_tx, history: ChatComposerHistory::new(), @@ -76,13 +87,13 @@ impl ChatComposer<'_> { dismissed_file_popup_token: None, current_file_query: None, pending_pastes: Vec::new(), - }; - this.update_border(has_input_focus); - this + token_usage_info: None, + has_focus: has_input_focus, + } } - pub fn desired_height(&self) -> u16 { - self.textarea.lines().len().max(1) as u16 + pub fn desired_height(&self, width: u16) -> u16 { + self.textarea.desired_height(width - 1) + match &self.active_popup { ActivePopup::None => 1u16, ActivePopup::Command(c) => c.calculate_required_height(), @@ -90,6 +101,21 @@ impl ChatComposer<'_> { } } + pub fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { + let popup_height = match &self.active_popup { + ActivePopup::Command(popup) => popup.calculate_required_height(), + ActivePopup::File(popup) => popup.calculate_required_height(), + ActivePopup::None => 1, + }; + let [textarea_rect, _] = + Layout::vertical([Constraint::Min(0), Constraint::Max(popup_height)]).areas(area); + let mut textarea_rect = textarea_rect; + textarea_rect.width = textarea_rect.width.saturating_sub(1); + textarea_rect.x += 1; + let state = self.textarea_state.borrow(); + self.textarea.cursor_pos_with_state(textarea_rect, &state) + } + /// Returns true if the composer currently contains no user input. pub(crate) fn is_empty(&self) -> bool { self.textarea.is_empty() @@ -103,28 +129,10 @@ impl ChatComposer<'_> { token_usage: TokenUsage, model_context_window: Option, ) { - let placeholder = match (token_usage.total_tokens, model_context_window) { - (total_tokens, Some(context_window)) => { - let percent_remaining: u8 = if context_window > 0 { - // Calculate the percentage of context left. - let percent = 100.0 - (total_tokens as f32 / context_window as f32 * 100.0); - percent.clamp(0.0, 100.0) as u8 - } else { - // If we don't have a context window, we cannot compute the - // percentage. - 100 - }; - // When https://github.com/openai/codex/issues/1257 is resolved, - // check if `percent_remaining < 25`, and if so, recommend - // /compact. - format!("{BASE_PLACEHOLDER_TEXT} — {percent_remaining}% context left") - } - (total_tokens, None) => { - format!("{BASE_PLACEHOLDER_TEXT} — {total_tokens} tokens used") - } - }; - - self.textarea.set_placeholder_text(placeholder); + self.token_usage_info = Some(TokenUsageInfo { + token_usage, + model_context_window, + }); } /// Record the history metadata advertised by `SessionConfiguredEvent` so @@ -142,8 +150,12 @@ impl ChatComposer<'_> { offset: usize, entry: Option, ) -> bool { - self.history - .on_entry_response(log_id, offset, entry, &mut self.textarea) + let Some(text) = self.history.on_entry_response(log_id, offset, entry) else { + return false; + }; + self.textarea.set_text(&text); + self.textarea.set_cursor(0); + true } pub fn handle_paste(&mut self, pasted: String) -> bool { @@ -179,7 +191,7 @@ impl ChatComposer<'_> { pub fn set_ctrl_c_quit_hint(&mut self, show: bool, has_focus: bool) { self.ctrl_c_quit_hint = show; - self.update_border(has_focus); + self.set_has_focus(has_focus); } /// Handle a key event coming from the main UI. @@ -207,49 +219,47 @@ impl ChatComposer<'_> { unreachable!(); }; - match key_event.into() { - Input { key: Key::Up, .. } => { + match key_event { + KeyEvent { + code: KeyCode::Up, .. + } => { popup.move_up(); (InputResult::None, true) } - Input { key: Key::Down, .. } => { + KeyEvent { + code: KeyCode::Down, + .. + } => { popup.move_down(); (InputResult::None, true) } - Input { key: Key::Tab, .. } => { + KeyEvent { + code: KeyCode::Tab, .. + } => { if let Some(cmd) = popup.selected_command() { - let first_line = self - .textarea - .lines() - .first() - .map(|s| s.as_str()) - .unwrap_or(""); + let first_line = self.textarea.text().lines().next().unwrap_or(""); let starts_with_cmd = first_line .trim_start() .starts_with(&format!("/{}", cmd.command())); if !starts_with_cmd { - self.textarea.select_all(); - self.textarea.cut(); - let _ = self.textarea.insert_str(format!("/{} ", cmd.command())); + self.textarea.set_text(&format!("/{} ", cmd.command())); } } (InputResult::None, true) } - Input { - key: Key::Enter, - shift: false, - alt: false, - ctrl: false, + KeyEvent { + code: KeyCode::Enter, + modifiers: KeyModifiers::NONE, + .. } => { if let Some(cmd) = popup.selected_command() { // Send command to the app layer. self.app_event_tx.send(AppEvent::DispatchCommand(*cmd)); // Clear textarea so no residual text remains. - self.textarea.select_all(); - self.textarea.cut(); + self.textarea.set_text(""); // Hide popup since the command has been dispatched. self.active_popup = ActivePopup::None; @@ -268,16 +278,23 @@ impl ChatComposer<'_> { unreachable!(); }; - match key_event.into() { - Input { key: Key::Up, .. } => { + match key_event { + KeyEvent { + code: KeyCode::Up, .. + } => { popup.move_up(); (InputResult::None, true) } - Input { key: Key::Down, .. } => { + KeyEvent { + code: KeyCode::Down, + .. + } => { popup.move_down(); (InputResult::None, true) } - Input { key: Key::Esc, .. } => { + KeyEvent { + code: KeyCode::Esc, .. + } => { // Hide popup without modifying text, remember token to avoid immediate reopen. if let Some(tok) = Self::current_at_token(&self.textarea) { self.dismissed_file_popup_token = Some(tok.to_string()); @@ -285,12 +302,13 @@ impl ChatComposer<'_> { self.active_popup = ActivePopup::None; (InputResult::None, true) } - Input { key: Key::Tab, .. } - | Input { - key: Key::Enter, - ctrl: false, - alt: false, - shift: false, + KeyEvent { + code: KeyCode::Tab, .. + } + | KeyEvent { + code: KeyCode::Enter, + modifiers: KeyModifiers::NONE, + .. } => { if let Some(sel) = popup.selected_match() { let sel_path = sel.to_string(); @@ -315,46 +333,89 @@ impl ChatComposer<'_> { /// - A token is delimited by ASCII whitespace (space, tab, newline). /// - If the token under the cursor starts with `@` and contains at least /// one additional character, that token (without `@`) is returned. - fn current_at_token(textarea: &tui_textarea::TextArea) -> Option { - let (row, col) = textarea.cursor(); + fn current_at_token(textarea: &TextArea) -> Option { + let cursor_offset = textarea.cursor(); + let text = textarea.text(); - // Guard against out-of-bounds rows. - let line = textarea.lines().get(row)?.as_str(); + // Adjust the provided byte offset to the nearest valid char boundary at or before it. + let mut safe_cursor = cursor_offset.min(text.len()); + // If we're not on a char boundary, move back to the start of the current char. + if safe_cursor < text.len() && !text.is_char_boundary(safe_cursor) { + // Find the last valid boundary <= cursor_offset. + safe_cursor = text + .char_indices() + .map(|(i, _)| i) + .take_while(|&i| i <= cursor_offset) + .last() + .unwrap_or(0); + } - // Calculate byte offset for cursor position - let cursor_byte_offset = line.chars().take(col).map(|c| c.len_utf8()).sum::(); + // Split the line around the (now safe) cursor position. + let before_cursor = &text[..safe_cursor]; + let after_cursor = &text[safe_cursor..]; - // Split the line at the cursor position so we can search for word - // boundaries on both sides. - let before_cursor = &line[..cursor_byte_offset]; - let after_cursor = &line[cursor_byte_offset..]; + // Detect whether we're on whitespace at the cursor boundary. + let at_whitespace = if safe_cursor < text.len() { + text[safe_cursor..] + .chars() + .next() + .map(|c| c.is_whitespace()) + .unwrap_or(false) + } else { + false + }; - // Find start index (first character **after** the previous multi-byte whitespace). - let start_idx = before_cursor + // Left candidate: token containing the cursor position. + let start_left = before_cursor .char_indices() .rfind(|(_, c)| c.is_whitespace()) .map(|(idx, c)| idx + c.len_utf8()) .unwrap_or(0); - - // Find end index (first multi-byte whitespace **after** the cursor position). - let end_rel_idx = after_cursor + let end_left_rel = after_cursor .char_indices() .find(|(_, c)| c.is_whitespace()) .map(|(idx, _)| idx) .unwrap_or(after_cursor.len()); - let end_idx = cursor_byte_offset + end_rel_idx; - - if start_idx >= end_idx { - return None; - } - - let token = &line[start_idx..end_idx]; - - if token.starts_with('@') && token.len() > 1 { - Some(token[1..].to_string()) + let end_left = safe_cursor + end_left_rel; + let token_left = if start_left < end_left { + Some(&text[start_left..end_left]) } else { None + }; + + // Right candidate: token immediately after any whitespace from the cursor. + let ws_len_right: usize = after_cursor + .chars() + .take_while(|c| c.is_whitespace()) + .map(|c| c.len_utf8()) + .sum(); + let start_right = safe_cursor + ws_len_right; + let end_right_rel = text[start_right..] + .char_indices() + .find(|(_, c)| c.is_whitespace()) + .map(|(idx, _)| idx) + .unwrap_or(text.len() - start_right); + let end_right = start_right + end_right_rel; + let token_right = if start_right < end_right { + Some(&text[start_right..end_right]) + } else { + None + }; + + let left_at = token_left + .filter(|t| t.starts_with('@') && t.len() > 1) + .map(|t| t[1..].to_string()); + let right_at = token_right + .filter(|t| t.starts_with('@') && t.len() > 1) + .map(|t| t[1..].to_string()); + + if at_whitespace { + return right_at.or(left_at); } + if after_cursor.starts_with('@') { + return right_at.or(left_at); + } + left_at.or(right_at) } /// Replace the active `@token` (the one under the cursor) with `path`. @@ -363,94 +424,73 @@ impl ChatComposer<'_> { /// where the cursor is within the token and regardless of how many /// `@tokens` exist in the line. fn insert_selected_path(&mut self, path: &str) { - let (row, col) = self.textarea.cursor(); + let cursor_offset = self.textarea.cursor(); + let text = self.textarea.text(); - // Materialize the textarea lines so we can mutate them easily. - let mut lines: Vec = self.textarea.lines().to_vec(); + let before_cursor = &text[..cursor_offset]; + let after_cursor = &text[cursor_offset..]; - if let Some(line) = lines.get_mut(row) { - // Calculate byte offset for cursor position - let cursor_byte_offset = line.chars().take(col).map(|c| c.len_utf8()).sum::(); + // Determine token boundaries. + let start_idx = before_cursor + .char_indices() + .rfind(|(_, c)| c.is_whitespace()) + .map(|(idx, c)| idx + c.len_utf8()) + .unwrap_or(0); - let before_cursor = &line[..cursor_byte_offset]; - let after_cursor = &line[cursor_byte_offset..]; + let end_rel_idx = after_cursor + .char_indices() + .find(|(_, c)| c.is_whitespace()) + .map(|(idx, _)| idx) + .unwrap_or(after_cursor.len()); + let end_idx = cursor_offset + end_rel_idx; - // Determine token boundaries. - let start_idx = before_cursor - .char_indices() - .rfind(|(_, c)| c.is_whitespace()) - .map(|(idx, c)| idx + c.len_utf8()) - .unwrap_or(0); + // Replace the slice `[start_idx, end_idx)` with the chosen path and a trailing space. + let mut new_text = + String::with_capacity(text.len() - (end_idx - start_idx) + path.len() + 1); + new_text.push_str(&text[..start_idx]); + new_text.push_str(path); + new_text.push(' '); + new_text.push_str(&text[end_idx..]); - let end_rel_idx = after_cursor - .char_indices() - .find(|(_, c)| c.is_whitespace()) - .map(|(idx, _)| idx) - .unwrap_or(after_cursor.len()); - let end_idx = cursor_byte_offset + end_rel_idx; - - // Replace the slice `[start_idx, end_idx)` with the chosen path and a trailing space. - let mut new_line = - String::with_capacity(line.len() - (end_idx - start_idx) + path.len() + 1); - new_line.push_str(&line[..start_idx]); - new_line.push_str(path); - new_line.push(' '); - new_line.push_str(&line[end_idx..]); - - *line = new_line; - - // Re-populate the textarea. - let new_text = lines.join("\n"); - self.textarea.select_all(); - self.textarea.cut(); - let _ = self.textarea.insert_str(new_text); - - // Note: tui-textarea currently exposes only relative cursor - // movements. Leaving the cursor position unchanged is acceptable - // as subsequent typing will move the cursor naturally. - } + self.textarea.set_text(&new_text); } /// Handle key event when no popup is visible. fn handle_key_event_without_popup(&mut self, key_event: KeyEvent) -> (InputResult, bool) { - let input: Input = key_event.into(); - match input { + match key_event { // ------------------------------------------------------------- // History navigation (Up / Down) – only when the composer is not // empty or when the cursor is at the correct position, to avoid // interfering with normal cursor movement. // ------------------------------------------------------------- - Input { key: Key::Up, .. } => { - if self.history.should_handle_navigation(&self.textarea) { - let consumed = self - .history - .navigate_up(&mut self.textarea, &self.app_event_tx); - if consumed { - return (InputResult::None, true); - } - } - self.handle_input_basic(input) - } - Input { key: Key::Down, .. } => { - if self.history.should_handle_navigation(&self.textarea) { - let consumed = self - .history - .navigate_down(&mut self.textarea, &self.app_event_tx); - if consumed { - return (InputResult::None, true); - } - } - self.handle_input_basic(input) - } - Input { - key: Key::Enter, - shift: false, - alt: false, - ctrl: false, + KeyEvent { + code: KeyCode::Up | KeyCode::Down, + .. } => { - let mut text = self.textarea.lines().join("\n"); - self.textarea.select_all(); - self.textarea.cut(); + if self + .history + .should_handle_navigation(self.textarea.text(), self.textarea.cursor()) + { + let replace_text = match key_event.code { + KeyCode::Up => self.history.navigate_up(&self.app_event_tx), + KeyCode::Down => self.history.navigate_down(&self.app_event_tx), + _ => unreachable!(), + }; + if let Some(text) = replace_text { + self.textarea.set_text(&text); + self.textarea.set_cursor(0); + return (InputResult::None, true); + } + } + self.handle_input_basic(key_event) + } + KeyEvent { + code: KeyCode::Enter, + modifiers: KeyModifiers::NONE, + .. + } => { + let mut text = self.textarea.text().to_string(); + self.textarea.set_text(""); // Replace all pending pastes in the text for (placeholder, actual) in &self.pending_pastes { @@ -467,41 +507,15 @@ impl ChatComposer<'_> { (InputResult::Submitted(text), true) } } - Input { - key: Key::Enter, .. - } - | Input { - key: Key::Char('j'), - ctrl: true, - alt: false, - shift: false, - } => { - 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), } } /// Handle generic Input events that modify the textarea content. - fn handle_input_basic(&mut self, input: Input) -> (InputResult, bool) { + fn handle_input_basic(&mut self, input: KeyEvent) -> (InputResult, bool) { // Special handling for backspace on placeholders - if let Input { - key: Key::Backspace, + if let KeyEvent { + code: KeyCode::Backspace, .. } = input { @@ -510,20 +524,9 @@ impl ChatComposer<'_> { } } - if let Input { - key: Key::Char('u'), - ctrl: true, - alt: false, - .. - } = input - { - self.textarea.delete_line_by_head(); - return (InputResult::None, true); - } - // Normal input handling self.textarea.input(input); - let text_after = self.textarea.lines().join("\n"); + let text_after = self.textarea.text(); // Check if any placeholders were removed and remove their corresponding pending pastes self.pending_pastes @@ -535,21 +538,16 @@ impl ChatComposer<'_> { /// Attempts to remove a placeholder if the cursor is at the end of one. /// Returns true if a placeholder was removed. fn try_remove_placeholder_at_cursor(&mut self) -> bool { - let (row, col) = self.textarea.cursor(); - let line = self - .textarea - .lines() - .get(row) - .map(|s| s.as_str()) - .unwrap_or(""); + let p = self.textarea.cursor(); + let text = self.textarea.text(); // Find any placeholder that ends at the cursor position let placeholder_to_remove = self.pending_pastes.iter().find_map(|(ph, _)| { - if col < ph.len() { + if p < ph.len() { return None; } - let potential_ph_start = col - ph.len(); - if line[potential_ph_start..col] == *ph { + let potential_ph_start = p - ph.len(); + if text[potential_ph_start..p] == *ph { Some(ph.clone()) } else { None @@ -557,17 +555,7 @@ impl ChatComposer<'_> { }); if let Some(placeholder) = placeholder_to_remove { - // Remove the entire placeholder from the text - let placeholder_len = placeholder.len(); - for _ in 0..placeholder_len { - self.textarea.input(Input { - key: Key::Backspace, - ctrl: false, - alt: false, - shift: false, - }); - } - // Remove from pending pastes + self.textarea.replace_range(p - placeholder.len()..p, ""); self.pending_pastes.retain(|(ph, _)| ph != &placeholder); true } else { @@ -579,16 +567,7 @@ impl ChatComposer<'_> { /// textarea. This must be called after every modification that can change /// the text so the popup is shown/updated/hidden as appropriate. fn sync_command_popup(&mut self) { - // Inspect only the first line to decide whether to show the popup. In - // the common case (no leading slash) we avoid copying the entire - // textarea contents. - let first_line = self - .textarea - .lines() - .first() - .map(|s| s.as_str()) - .unwrap_or(""); - + let first_line = self.textarea.text().lines().next().unwrap_or(""); let input_starts_with_slash = first_line.starts_with('/'); match &mut self.active_popup { ActivePopup::Command(popup) => { @@ -644,74 +623,29 @@ impl ChatComposer<'_> { self.dismissed_file_popup_token = None; } - fn update_border(&mut self, has_focus: bool) { - let border_style = if has_focus { - Style::default().fg(Color::Cyan) - } else { - Style::default().dim() - }; - - self.textarea.set_block( - ratatui::widgets::Block::default() - .borders(Borders::LEFT) - .border_type(BorderType::QuadrantOutside) - .border_style(border_style), - ); + fn set_has_focus(&mut self, has_focus: bool) { + self.has_focus = has_focus; } } -impl WidgetRef for &ChatComposer<'_> { +impl WidgetRef for &ChatComposer { fn render_ref(&self, area: Rect, buf: &mut Buffer) { + let popup_height = match &self.active_popup { + ActivePopup::Command(popup) => popup.calculate_required_height(), + ActivePopup::File(popup) => popup.calculate_required_height(), + ActivePopup::None => 1, + }; + let [textarea_rect, popup_rect] = + Layout::vertical([Constraint::Min(0), Constraint::Max(popup_height)]).areas(area); match &self.active_popup { ActivePopup::Command(popup) => { - let popup_height = popup.calculate_required_height(); - - // Split the provided rect so that the popup is rendered at the - // **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: 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); + popup.render_ref(popup_rect, buf); } ActivePopup::File(popup) => { - let popup_height = popup.calculate_required_height(); - - let popup_height = popup_height.min(area.height); - let textarea_rect = Rect { - x: area.x, - y: area.y, - 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); + popup.render_ref(popup_rect, buf); } ActivePopup::None => { - 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 bottom_line_rect = popup_rect; let key_hint_style = Style::default().fg(Color::Cyan); let hint = if self.ctrl_c_quit_hint { vec![ @@ -740,6 +674,56 @@ impl WidgetRef for &ChatComposer<'_> { .render_ref(bottom_line_rect, buf); } } + Block::default() + .border_style(Style::default().dim()) + .borders(Borders::LEFT) + .border_type(BorderType::QuadrantOutside) + .border_style(Style::default().fg(if self.has_focus { + Color::Cyan + } else { + Color::Gray + })) + .render_ref( + Rect::new(textarea_rect.x, textarea_rect.y, 1, textarea_rect.height), + buf, + ); + let mut textarea_rect = textarea_rect; + textarea_rect.width = textarea_rect.width.saturating_sub(1); + textarea_rect.x += 1; + let mut state = self.textarea_state.borrow_mut(); + StatefulWidgetRef::render_ref(&(&self.textarea), textarea_rect, buf, &mut state); + if self.textarea.text().is_empty() { + let placeholder = if let Some(token_usage_info) = &self.token_usage_info { + let token_usage = &token_usage_info.token_usage; + let model_context_window = token_usage_info.model_context_window; + match (token_usage.total_tokens, model_context_window) { + (total_tokens, Some(context_window)) => { + let percent_remaining: u8 = if context_window > 0 { + // Calculate the percentage of context left. + let percent = + 100.0 - (total_tokens as f32 / context_window as f32 * 100.0); + percent.clamp(0.0, 100.0) as u8 + } else { + // If we don't have a context window, we cannot compute the + // percentage. + 100 + }; + // When https://github.com/openai/codex/issues/1257 is resolved, + // check if `percent_remaining < 25`, and if so, recommend + // /compact. + format!("{BASE_PLACEHOLDER_TEXT} — {percent_remaining}% context left") + } + (total_tokens, None) => { + format!("{BASE_PLACEHOLDER_TEXT} — {total_tokens} tokens used") + } + } + } else { + BASE_PLACEHOLDER_TEXT.to_string() + }; + Line::from(placeholder) + .style(Style::default().dim()) + .render_ref(textarea_rect.inner(Margin::new(1, 0)), buf); + } } } @@ -749,7 +733,7 @@ mod tests { use crate::bottom_pane::ChatComposer; use crate::bottom_pane::InputResult; use crate::bottom_pane::chat_composer::LARGE_PASTE_CHAR_THRESHOLD; - use tui_textarea::TextArea; + use crate::bottom_pane::textarea::TextArea; #[test] fn test_current_at_token_basic_cases() { @@ -792,9 +776,9 @@ mod tests { ]; for (input, cursor_pos, expected, description) in test_cases { - let mut textarea = TextArea::default(); + let mut textarea = TextArea::new(); textarea.insert_str(input); - textarea.move_cursor(tui_textarea::CursorMove::Jump(0, cursor_pos)); + textarea.set_cursor(cursor_pos); let result = ChatComposer::current_at_token(&textarea); assert_eq!( @@ -826,9 +810,9 @@ mod tests { ]; for (input, cursor_pos, expected, description) in test_cases { - let mut textarea = TextArea::default(); + let mut textarea = TextArea::new(); textarea.insert_str(input); - textarea.move_cursor(tui_textarea::CursorMove::Jump(0, cursor_pos)); + textarea.set_cursor(cursor_pos); let result = ChatComposer::current_at_token(&textarea); assert_eq!( @@ -863,13 +847,13 @@ mod tests { // Full-width space boundaries ( "test　@İstanbul", - 6, + 8, Some("İstanbul".to_string()), "@ token after full-width space", ), ( "@ЙЦУ　@诶", - 6, + 10, Some("诶".to_string()), "Full-width space between Unicode tokens", ), @@ -883,9 +867,9 @@ mod tests { ]; for (input, cursor_pos, expected, description) in test_cases { - let mut textarea = TextArea::default(); + let mut textarea = TextArea::new(); textarea.insert_str(input); - textarea.move_cursor(tui_textarea::CursorMove::Jump(0, cursor_pos)); + textarea.set_cursor(cursor_pos); let result = ChatComposer::current_at_token(&textarea); assert_eq!( @@ -907,7 +891,7 @@ mod tests { let needs_redraw = composer.handle_paste("hello".to_string()); assert!(needs_redraw); - assert_eq!(composer.textarea.lines(), ["hello"]); + assert_eq!(composer.textarea.text(), "hello"); assert!(composer.pending_pastes.is_empty()); let (result, _) = @@ -932,7 +916,7 @@ mod tests { let needs_redraw = composer.handle_paste(large.clone()); assert!(needs_redraw); let placeholder = format!("[Pasted Content {} chars]", large.chars().count()); - assert_eq!(composer.textarea.lines(), [placeholder.as_str()]); + assert_eq!(composer.textarea.text(), placeholder); assert_eq!(composer.pending_pastes.len(), 1); assert_eq!(composer.pending_pastes[0].0, placeholder); assert_eq!(composer.pending_pastes[0].1, large); @@ -1008,7 +992,7 @@ mod tests { composer.handle_paste("b".repeat(LARGE_PASTE_CHAR_THRESHOLD + 4)); composer.handle_paste("c".repeat(LARGE_PASTE_CHAR_THRESHOLD + 6)); // Move cursor to end and press backspace - composer.textarea.move_cursor(tui_textarea::CursorMove::End); + composer.textarea.set_cursor(composer.textarea.text().len()); composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); } @@ -1123,7 +1107,7 @@ mod tests { current_pos += content.len(); } ( - composer.textarea.lines().join("\n"), + composer.textarea.text().to_string(), composer.pending_pastes.len(), current_pos, ) @@ -1134,25 +1118,18 @@ mod tests { let mut deletion_states = vec![]; // First deletion - composer - .textarea - .move_cursor(tui_textarea::CursorMove::Jump(0, states[0].2 as u16)); + composer.textarea.set_cursor(states[0].2); composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); deletion_states.push(( - composer.textarea.lines().join("\n"), + composer.textarea.text().to_string(), composer.pending_pastes.len(), )); // Second deletion - composer - .textarea - .move_cursor(tui_textarea::CursorMove::Jump( - 0, - composer.textarea.lines().join("\n").len() as u16, - )); + composer.textarea.set_cursor(composer.textarea.text().len()); composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); deletion_states.push(( - composer.textarea.lines().join("\n"), + composer.textarea.text().to_string(), composer.pending_pastes.len(), )); @@ -1191,17 +1168,13 @@ mod tests { composer.handle_paste(paste.clone()); composer .textarea - .move_cursor(tui_textarea::CursorMove::Jump( - 0, - (placeholder.len() - pos_from_end) as u16, - )); + .set_cursor((placeholder.len() - pos_from_end) as usize); composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); let result = ( - composer.textarea.lines().join("\n").contains(&placeholder), + composer.textarea.text().contains(&placeholder), composer.pending_pastes.len(), ); - composer.textarea.select_all(); - composer.textarea.cut(); + composer.textarea.set_text(""); result }) .collect(); diff --git a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs index 5715c99492..a744a409f3 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs @@ -1,8 +1,5 @@ use std::collections::HashMap; -use tui_textarea::CursorMove; -use tui_textarea::TextArea; - use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use codex_core::protocol::Op; @@ -67,59 +64,52 @@ impl ChatComposerHistory { /// Should Up/Down key presses be interpreted as history navigation given /// the current content and cursor position of `textarea`? - pub fn should_handle_navigation(&self, textarea: &TextArea) -> bool { + pub fn should_handle_navigation(&self, text: &str, cursor: usize) -> bool { if self.history_entry_count == 0 && self.local_history.is_empty() { return false; } - if textarea.is_empty() { + if text.is_empty() { return true; } // Textarea is not empty – only navigate when cursor is at start and // text matches last recalled history entry so regular editing is not // hijacked. - let (row, col) = textarea.cursor(); - if row != 0 || col != 0 { + if cursor != 0 { return false; } - let lines = textarea.lines(); - matches!(&self.last_history_text, Some(prev) if prev == &lines.join("\n")) + matches!(&self.last_history_text, Some(prev) if prev == text) } /// Handle . Returns true when the key was consumed and the caller /// should request a redraw. - pub fn navigate_up(&mut self, textarea: &mut TextArea, app_event_tx: &AppEventSender) -> bool { + pub fn navigate_up(&mut self, app_event_tx: &AppEventSender) -> Option { let total_entries = self.history_entry_count + self.local_history.len(); if total_entries == 0 { - return false; + return None; } let next_idx = match self.history_cursor { None => (total_entries as isize) - 1, - Some(0) => return true, // already at oldest + Some(0) => return None, // already at oldest Some(idx) => idx - 1, }; self.history_cursor = Some(next_idx); - self.populate_history_at_index(next_idx as usize, textarea, app_event_tx); - true + self.populate_history_at_index(next_idx as usize, app_event_tx) } /// Handle . - pub fn navigate_down( - &mut self, - textarea: &mut TextArea, - app_event_tx: &AppEventSender, - ) -> bool { + pub fn navigate_down(&mut self, app_event_tx: &AppEventSender) -> Option { let total_entries = self.history_entry_count + self.local_history.len(); if total_entries == 0 { - return false; + return None; } let next_idx_opt = match self.history_cursor { - None => return false, // not browsing + None => return None, // not browsing Some(idx) if (idx as usize) + 1 >= total_entries => None, Some(idx) => Some(idx + 1), }; @@ -127,16 +117,15 @@ impl ChatComposerHistory { match next_idx_opt { Some(idx) => { self.history_cursor = Some(idx); - self.populate_history_at_index(idx as usize, textarea, app_event_tx); + self.populate_history_at_index(idx as usize, app_event_tx) } None => { // Past newest – clear and exit browsing mode. self.history_cursor = None; self.last_history_text = None; - self.replace_textarea_content(textarea, ""); + Some(String::new()) } } - true } /// Integrate a GetHistoryEntryResponse event. @@ -145,19 +134,18 @@ impl ChatComposerHistory { log_id: u64, offset: usize, entry: Option, - textarea: &mut TextArea, - ) -> bool { + ) -> Option { if self.history_log_id != Some(log_id) { - return false; + return None; } - let Some(text) = entry else { return false }; + let text = entry?; self.fetched_history.insert(offset, text.clone()); if self.history_cursor == Some(offset as isize) { - self.replace_textarea_content(textarea, &text); - return true; + self.last_history_text = Some(text.clone()); + return Some(text); } - false + None } // --------------------------------------------------------------------- @@ -167,21 +155,20 @@ impl ChatComposerHistory { fn populate_history_at_index( &mut self, global_idx: usize, - textarea: &mut TextArea, app_event_tx: &AppEventSender, - ) { + ) -> Option { if global_idx >= self.history_entry_count { // Local entry. if let Some(text) = self .local_history .get(global_idx - self.history_entry_count) { - let t = text.clone(); - self.replace_textarea_content(textarea, &t); + self.last_history_text = Some(text.clone()); + return Some(text.clone()); } } else if let Some(text) = self.fetched_history.get(&global_idx) { - let t = text.clone(); - self.replace_textarea_content(textarea, &t); + self.last_history_text = Some(text.clone()); + return Some(text.clone()); } else if let Some(log_id) = self.history_log_id { let op = Op::GetHistoryEntryRequest { offset: global_idx, @@ -189,14 +176,7 @@ impl ChatComposerHistory { }; app_event_tx.send(AppEvent::CodexOp(op)); } - } - - fn replace_textarea_content(&mut self, textarea: &mut TextArea, text: &str) { - textarea.select_all(); - textarea.cut(); - let _ = textarea.insert_str(text); - textarea.move_cursor(CursorMove::Jump(0, 0)); - self.last_history_text = Some(text.to_string()); + None } } @@ -217,11 +197,9 @@ mod tests { // Pretend there are 3 persistent entries. history.set_metadata(1, 3); - let mut textarea = TextArea::default(); - // First Up should request offset 2 (latest) and await async data. - assert!(history.should_handle_navigation(&textarea)); - assert!(history.navigate_up(&mut textarea, &tx)); + assert!(history.should_handle_navigation("", 0)); + assert!(history.navigate_up(&tx).is_none()); // don't replace the text yet // Verify that an AppEvent::CodexOp with the correct GetHistoryEntryRequest was sent. let event = rx.try_recv().expect("expected AppEvent to be sent"); @@ -235,14 +213,15 @@ mod tests { }, history_request1 ); - assert_eq!(textarea.lines().join("\n"), ""); // still empty // Inject the async response. - assert!(history.on_entry_response(1, 2, Some("latest".into()), &mut textarea)); - assert_eq!(textarea.lines().join("\n"), "latest"); + assert_eq!( + Some("latest".into()), + history.on_entry_response(1, 2, Some("latest".into())) + ); // Next Up should move to offset 1. - assert!(history.navigate_up(&mut textarea, &tx)); + assert!(history.navigate_up(&tx).is_none()); // don't replace the text yet // Verify second CodexOp event for offset 1. let event2 = rx.try_recv().expect("expected second event"); @@ -257,7 +236,9 @@ mod tests { history_request_2 ); - history.on_entry_response(1, 1, Some("older".into()), &mut textarea); - assert_eq!(textarea.lines().join("\n"), "older"); + assert_eq!( + Some("older".into()), + history.on_entry_response(1, 1, Some("older".into())) + ); } } diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 281f0859a5..cab78bbe3f 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -19,6 +19,7 @@ mod chat_composer_history; mod command_popup; mod file_search_popup; mod status_indicator_view; +mod textarea; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum CancellationEvent { @@ -36,7 +37,7 @@ use status_indicator_view::StatusIndicatorView; pub(crate) struct BottomPane<'a> { /// Composer is retained even when a BottomPaneView is displayed so the /// input state is retained when the view is closed. - composer: ChatComposer<'a>, + composer: ChatComposer, /// If present, this is displayed instead of the `composer`. active_view: Option + 'a>>, @@ -74,7 +75,19 @@ impl BottomPane<'_> { self.active_view .as_ref() .map(|v| v.desired_height(width)) - .unwrap_or(self.composer.desired_height()) + .unwrap_or(self.composer.desired_height(width)) + } + + pub fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { + // Hide the cursor whenever an overlay view is active (e.g. the + // status indicator shown while a task is running, or approval modal). + // In these states the textarea is not interactable, so we should not + // show its caret. + if self.active_view.is_some() { + None + } else { + self.composer.cursor_pos(area) + } } /// Forward a key event to the active view or the composer. diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs new file mode 100644 index 0000000000..e150135b75 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -0,0 +1,1294 @@ +use crossterm::event::KeyCode; +use crossterm::event::KeyEvent; +use crossterm::event::KeyModifiers; +use ratatui::buffer::Buffer; +use ratatui::layout::Rect; +use ratatui::style::Style; +use ratatui::widgets::StatefulWidgetRef; +use ratatui::widgets::WidgetRef; +use std::cell::Ref; +use std::cell::RefCell; +use std::ops::Range; +use textwrap::Options; +use unicode_segmentation::UnicodeSegmentation; +use unicode_width::UnicodeWidthStr; + +#[derive(Debug)] +pub(crate) struct TextArea { + text: String, + cursor_pos: usize, + wrap_cache: RefCell>, + preferred_col: Option, +} + +#[derive(Debug, Clone)] +struct WrapCache { + width: u16, + lines: Vec>, +} + +#[derive(Debug, Default, Clone, Copy)] +pub(crate) struct TextAreaState { + /// Index into wrapped lines of the first visible line. + scroll: u16, +} + +impl TextArea { + pub fn new() -> Self { + Self { + text: String::new(), + cursor_pos: 0, + wrap_cache: RefCell::new(None), + preferred_col: None, + } + } + + pub fn set_text(&mut self, text: &str) { + self.text = text.to_string(); + self.cursor_pos = self.cursor_pos.clamp(0, self.text.len()); + self.wrap_cache.replace(None); + self.preferred_col = None; + } + + pub fn text(&self) -> &str { + &self.text + } + + pub fn insert_str(&mut self, text: &str) { + self.insert_str_at(self.cursor_pos, text); + } + + pub fn insert_str_at(&mut self, pos: usize, text: &str) { + self.text.insert_str(pos, text); + self.wrap_cache.replace(None); + if pos <= self.cursor_pos { + self.cursor_pos += text.len(); + } + self.preferred_col = None; + } + + pub fn replace_range(&mut self, range: std::ops::Range, text: &str) { + assert!(range.start <= range.end); + let start = range.start.clamp(0, self.text.len()); + let end = range.end.clamp(0, self.text.len()); + let removed_len = end - start; + let inserted_len = text.len(); + if removed_len == 0 && inserted_len == 0 { + return; + } + let diff = inserted_len as isize - removed_len as isize; + + self.text.replace_range(range, text); + self.wrap_cache.replace(None); + self.preferred_col = None; + + // Update the cursor position to account for the edit. + self.cursor_pos = if self.cursor_pos < start { + // Cursor was before the edited range – no shift. + self.cursor_pos + } else if self.cursor_pos <= end { + // Cursor was inside the replaced range – move to end of the new text. + start + inserted_len + } else { + // Cursor was after the replaced range – shift by the length diff. + ((self.cursor_pos as isize) + diff) as usize + } + .min(self.text.len()); + } + + pub fn cursor(&self) -> usize { + self.cursor_pos + } + + pub fn set_cursor(&mut self, pos: usize) { + self.cursor_pos = pos.clamp(0, self.text.len()); + self.preferred_col = None; + } + + pub fn desired_height(&self, width: u16) -> u16 { + self.wrapped_lines(width).len() as u16 + } + + #[allow(dead_code)] + pub fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { + self.cursor_pos_with_state(area, &TextAreaState::default()) + } + + /// Compute the on-screen cursor position taking scrolling into account. + pub fn cursor_pos_with_state(&self, area: Rect, state: &TextAreaState) -> Option<(u16, u16)> { + let lines = self.wrapped_lines(area.width); + let effective_scroll = self.effective_scroll(area.height, &lines, state.scroll); + let i = Self::wrapped_line_index_by_start(&lines, self.cursor_pos)?; + let ls = &lines[i]; + let col = self.text[ls.start..self.cursor_pos].width() as u16; + let screen_row = i + .saturating_sub(effective_scroll as usize) + .try_into() + .unwrap_or(0); + Some((area.x + col, area.y + screen_row)) + } + + pub fn is_empty(&self) -> bool { + self.text.is_empty() + } + + fn current_display_col(&self) -> usize { + let bol = self.beginning_of_current_line(); + self.text[bol..self.cursor_pos].width() + } + + fn wrapped_line_index_by_start(lines: &[Range], pos: usize) -> Option { + // partition_point returns the index of the first element for which + // the predicate is false, i.e. the count of elements with start <= pos. + let idx = lines.partition_point(|r| r.start <= pos); + if idx == 0 { None } else { Some(idx - 1) } + } + + fn move_to_display_col_on_line( + &mut self, + line_start: usize, + line_end: usize, + target_col: usize, + ) { + let mut width_so_far = 0usize; + for (i, g) in self.text[line_start..line_end].grapheme_indices(true) { + width_so_far += g.width(); + if width_so_far > target_col { + self.cursor_pos = line_start + i; + return; + } + } + self.cursor_pos = line_end; + } + + fn beginning_of_line(&self, pos: usize) -> usize { + self.text[..pos].rfind('\n').map(|i| i + 1).unwrap_or(0) + } + fn beginning_of_current_line(&self) -> usize { + self.beginning_of_line(self.cursor_pos) + } + + fn end_of_line(&self, pos: usize) -> usize { + self.text[pos..] + .find('\n') + .map(|i| i + pos) + .unwrap_or(self.text.len()) + } + fn end_of_current_line(&self) -> usize { + self.end_of_line(self.cursor_pos) + } + + pub(crate) fn beginning_of_previous_word(&self) -> usize { + if let Some(first_non_ws) = self.text[..self.cursor_pos].rfind(|c: char| !c.is_whitespace()) + { + self.text[..first_non_ws] + .rfind(|c: char| c.is_whitespace()) + .map(|i| i + 1) + .unwrap_or(0) + } else { + 0 + } + } + + pub(crate) fn end_of_next_word(&self) -> usize { + let Some(first_non_ws) = self.text[self.cursor_pos..].find(|c: char| !c.is_whitespace()) + else { + return self.text.len(); + }; + let word_start = self.cursor_pos + first_non_ws; + match self.text[word_start..].find(|c: char| c.is_whitespace()) { + Some(rel_idx) => word_start + rel_idx, + None => self.text.len(), + } + } + + pub fn input(&mut self, event: KeyEvent) { + match event { + KeyEvent { + code: KeyCode::Char(c), + modifiers: KeyModifiers::NONE | KeyModifiers::SHIFT | KeyModifiers::ALT, + .. + } => self.insert_str(&c.to_string()), + KeyEvent { + code: KeyCode::Char('j'), + modifiers: KeyModifiers::CONTROL, + .. + } + | KeyEvent { + code: KeyCode::Enter, + .. + } => self.insert_str("\n"), + KeyEvent { + code: KeyCode::Backspace, + .. + } => self.delete_backward(1), + KeyEvent { + code: KeyCode::Delete, + .. + } => self.delete_forward(1), + + KeyEvent { + code: KeyCode::Char('w'), + modifiers: KeyModifiers::CONTROL, + .. + } => { + self.delete_backward_word(); + } + KeyEvent { + code: KeyCode::Char('u'), + modifiers: KeyModifiers::CONTROL, + .. + } => { + self.kill_to_beginning_of_line(); + } + KeyEvent { + code: KeyCode::Char('k'), + modifiers: KeyModifiers::CONTROL, + .. + } => { + self.kill_to_end_of_line(); + } + + // Cursor movement + KeyEvent { + code: KeyCode::Left, + modifiers: KeyModifiers::NONE, + .. + } => { + self.move_cursor_left(); + } + KeyEvent { + code: KeyCode::Right, + modifiers: KeyModifiers::NONE, + .. + } => { + self.move_cursor_right(); + } + KeyEvent { + code: KeyCode::Up, .. + } => { + self.move_cursor_up(); + } + KeyEvent { + code: KeyCode::Down, + .. + } => { + self.move_cursor_down(); + } + KeyEvent { + code: KeyCode::Home, + .. + } => { + self.move_cursor_to_beginning_of_line(false); + } + KeyEvent { + code: KeyCode::Char('a'), + modifiers: KeyModifiers::CONTROL, + .. + } => { + self.move_cursor_to_beginning_of_line(true); + } + + KeyEvent { + code: KeyCode::End, .. + } => { + self.move_cursor_to_end_of_line(false); + } + KeyEvent { + code: KeyCode::Char('e'), + modifiers: KeyModifiers::CONTROL, + .. + } => { + self.move_cursor_to_end_of_line(true); + } + KeyEvent { + code: KeyCode::Left, + modifiers: KeyModifiers::CONTROL, + .. + } => { + self.set_cursor(self.beginning_of_previous_word()); + } + KeyEvent { + code: KeyCode::Right, + modifiers: KeyModifiers::CONTROL, + .. + } => { + self.set_cursor(self.end_of_next_word()); + } + o => { + tracing::debug!("Unhandled key event in TextArea: {:?}", o); + } + } + } + + // ####### Input Functions ####### + pub fn delete_backward(&mut self, n: usize) { + if n == 0 || self.cursor_pos == 0 { + return; + } + let mut gc = + unicode_segmentation::GraphemeCursor::new(self.cursor_pos, self.text.len(), false); + let mut target = self.cursor_pos; + for _ in 0..n { + match gc.prev_boundary(&self.text, 0) { + Ok(Some(b)) => target = b, + Ok(None) => { + target = 0; + break; + } + Err(_) => { + target = target.saturating_sub(1); + } + } + } + self.replace_range(target..self.cursor_pos, ""); + } + + pub fn delete_forward(&mut self, n: usize) { + if n == 0 || self.cursor_pos >= self.text.len() { + return; + } + let mut gc = + unicode_segmentation::GraphemeCursor::new(self.cursor_pos, self.text.len(), false); + let mut target = self.cursor_pos; + for _ in 0..n { + match gc.next_boundary(&self.text, 0) { + Ok(Some(b)) => target = b, + Ok(None) => { + target = self.text.len(); + break; + } + Err(_) => { + target = target.saturating_add(1); + } + } + } + self.replace_range(self.cursor_pos..target, ""); + } + + pub fn delete_backward_word(&mut self) { + self.replace_range(self.beginning_of_previous_word()..self.cursor_pos, ""); + } + + pub fn kill_to_end_of_line(&mut self) { + let eol = self.end_of_current_line(); + if self.cursor_pos == eol { + if eol < self.text.len() { + self.replace_range(self.cursor_pos..eol + 1, ""); + } + } else { + self.replace_range(self.cursor_pos..eol, ""); + } + } + + pub fn kill_to_beginning_of_line(&mut self) { + let bol = self.beginning_of_current_line(); + if self.cursor_pos == bol { + if bol > 0 { + self.replace_range(bol - 1..bol, ""); + } + } else { + self.replace_range(bol..self.cursor_pos, ""); + } + } + + /// Move the cursor left by a single grapheme cluster. + pub fn move_cursor_left(&mut self) { + let mut gc = + unicode_segmentation::GraphemeCursor::new(self.cursor_pos, self.text.len(), false); + match gc.prev_boundary(&self.text, 0) { + Ok(Some(boundary)) => self.cursor_pos = boundary, + Ok(None) => self.cursor_pos = 0, // Already at start. + Err(_) => self.cursor_pos = self.cursor_pos.saturating_sub(1), + } + self.preferred_col = None; + } + + /// Move the cursor right by a single grapheme cluster. + pub fn move_cursor_right(&mut self) { + let mut gc = + unicode_segmentation::GraphemeCursor::new(self.cursor_pos, self.text.len(), false); + match gc.next_boundary(&self.text, 0) { + Ok(Some(boundary)) => self.cursor_pos = boundary, + Ok(None) => self.cursor_pos = self.text.len(), // Already at end. + Err(_) => self.cursor_pos = self.cursor_pos.saturating_add(1), + } + self.preferred_col = None; + } + + pub fn move_cursor_up(&mut self) { + // If we have a wrapping cache, prefer navigating across wrapped (visual) lines. + if let Some((target_col, maybe_line)) = { + let cache_ref = self.wrap_cache.borrow(); + if let Some(cache) = cache_ref.as_ref() { + let lines = &cache.lines; + if let Some(idx) = Self::wrapped_line_index_by_start(lines, self.cursor_pos) { + let cur_range = &lines[idx]; + let target_col = self + .preferred_col + .unwrap_or_else(|| self.text[cur_range.start..self.cursor_pos].width()); + if idx > 0 { + let prev = &lines[idx - 1]; + let line_start = prev.start; + let line_end = prev.end.saturating_sub(1); + Some((target_col, Some((line_start, line_end)))) + } else { + Some((target_col, None)) + } + } else { + None + } + } else { + None + } + } { + // We had wrapping info. Apply movement accordingly. + match maybe_line { + Some((line_start, line_end)) => { + if self.preferred_col.is_none() { + self.preferred_col = Some(target_col); + } + self.move_to_display_col_on_line(line_start, line_end, target_col); + return; + } + None => { + // Already at first visual line -> move to start + self.cursor_pos = 0; + self.preferred_col = None; + return; + } + } + } + + // Fallback to logical line navigation if we don't have wrapping info yet. + if let Some(prev_nl) = self.text[..self.cursor_pos].rfind('\n') { + let target_col = match self.preferred_col { + Some(c) => c, + None => { + let c = self.current_display_col(); + self.preferred_col = Some(c); + c + } + }; + let prev_line_start = self.text[..prev_nl].rfind('\n').map(|i| i + 1).unwrap_or(0); + let prev_line_end = prev_nl; + self.move_to_display_col_on_line(prev_line_start, prev_line_end, target_col); + } else { + self.cursor_pos = 0; + self.preferred_col = None; + } + } + + pub fn move_cursor_down(&mut self) { + // If we have a wrapping cache, prefer navigating across wrapped (visual) lines. + if let Some((target_col, move_to_last)) = { + let cache_ref = self.wrap_cache.borrow(); + if let Some(cache) = cache_ref.as_ref() { + let lines = &cache.lines; + if let Some(idx) = Self::wrapped_line_index_by_start(lines, self.cursor_pos) { + let cur_range = &lines[idx]; + let target_col = self + .preferred_col + .unwrap_or_else(|| self.text[cur_range.start..self.cursor_pos].width()); + if idx + 1 < lines.len() { + let next = &lines[idx + 1]; + let line_start = next.start; + let line_end = next.end.saturating_sub(1); + Some((target_col, Some((line_start, line_end)))) + } else { + Some((target_col, None)) + } + } else { + None + } + } else { + None + } + } { + match move_to_last { + Some((line_start, line_end)) => { + if self.preferred_col.is_none() { + self.preferred_col = Some(target_col); + } + self.move_to_display_col_on_line(line_start, line_end, target_col); + return; + } + None => { + // Already on last visual line -> move to end + self.cursor_pos = self.text.len(); + self.preferred_col = None; + return; + } + } + } + + // Fallback to logical line navigation if we don't have wrapping info yet. + let target_col = match self.preferred_col { + Some(c) => c, + None => { + let c = self.current_display_col(); + self.preferred_col = Some(c); + c + } + }; + if let Some(next_nl) = self.text[self.cursor_pos..] + .find('\n') + .map(|i| i + self.cursor_pos) + { + let next_line_start = next_nl + 1; + let next_line_end = self.text[next_line_start..] + .find('\n') + .map(|i| i + next_line_start) + .unwrap_or(self.text.len()); + self.move_to_display_col_on_line(next_line_start, next_line_end, target_col); + } else { + self.cursor_pos = self.text.len(); + self.preferred_col = None; + } + } + + pub fn move_cursor_to_beginning_of_line(&mut self, move_up_at_bol: bool) { + let bol = self.beginning_of_current_line(); + if move_up_at_bol && self.cursor_pos == bol { + self.set_cursor(self.beginning_of_line(self.cursor_pos.saturating_sub(1))); + } else { + self.set_cursor(bol); + } + self.preferred_col = None; + } + + pub fn move_cursor_to_end_of_line(&mut self, move_down_at_eol: bool) { + let eol = self.end_of_current_line(); + if move_down_at_eol && self.cursor_pos == eol { + let next_pos = (self.cursor_pos.saturating_add(1)).min(self.text.len()); + self.set_cursor(self.end_of_line(next_pos)); + } else { + self.set_cursor(eol); + } + } + + #[allow(clippy::unwrap_used)] + fn wrapped_lines(&self, width: u16) -> Ref<'_, Vec>> { + // Ensure cache is ready (potentially mutably borrow, then drop) + { + let mut cache = self.wrap_cache.borrow_mut(); + let needs_recalc = match cache.as_ref() { + Some(c) => c.width != width, + None => true, + }; + if needs_recalc { + let mut lines: Vec> = Vec::new(); + for line in textwrap::wrap( + &self.text, + Options::new(width as usize).wrap_algorithm(textwrap::WrapAlgorithm::FirstFit), + ) + .iter() + { + match line { + std::borrow::Cow::Borrowed(slice) => { + let start = + unsafe { slice.as_ptr().offset_from(self.text.as_ptr()) as usize }; + let end = start + slice.len(); + let trailing_spaces = + self.text[end..].chars().take_while(|c| *c == ' ').count(); + lines.push(start..end + trailing_spaces + 1); + } + std::borrow::Cow::Owned(_) => unreachable!(), + } + } + *cache = Some(WrapCache { width, lines }); + } + } + + let cache = self.wrap_cache.borrow(); + Ref::map(cache, |c| &c.as_ref().unwrap().lines) + } + + /// Calculate the scroll offset that should be used to satisfy the + /// invariants given the current area size and wrapped lines. + /// + /// - Cursor is always on screen. + /// - No scrolling if content fits in the area. + fn effective_scroll( + &self, + area_height: u16, + lines: &[Range], + current_scroll: u16, + ) -> u16 { + let total_lines = lines.len() as u16; + if area_height >= total_lines { + return 0; + } + + // Where is the cursor within wrapped lines? Prefer assigning boundary positions + // (where pos equals the start of a wrapped line) to that later line. + let cursor_line_idx = + Self::wrapped_line_index_by_start(lines, self.cursor_pos).unwrap_or(0) as u16; + + let max_scroll = total_lines.saturating_sub(area_height); + let mut scroll = current_scroll.min(max_scroll); + + // Ensure cursor is visible within [scroll, scroll + area_height) + if cursor_line_idx < scroll { + scroll = cursor_line_idx; + } else if cursor_line_idx >= scroll + area_height { + scroll = cursor_line_idx + 1 - area_height; + } + scroll + } +} + +impl WidgetRef for &TextArea { + fn render_ref(&self, area: Rect, buf: &mut Buffer) { + let lines = self.wrapped_lines(area.width); + for (i, ls) in lines.iter().enumerate() { + let s = &self.text[ls.start..ls.end - 1]; + buf.set_string(area.x, area.y + i as u16, s, Style::default()); + } + } +} + +impl StatefulWidgetRef for &TextArea { + type State = TextAreaState; + + fn render_ref(&self, area: Rect, buf: &mut Buffer, state: &mut Self::State) { + let lines = self.wrapped_lines(area.width); + let scroll = self.effective_scroll(area.height, &lines, state.scroll); + state.scroll = scroll; + + let start = scroll as usize; + let end = (scroll + area.height).min(lines.len() as u16) as usize; + for (row, ls) in (start..end).enumerate() { + let r = &lines[ls]; + let s = &self.text[r.start..r.end - 1]; + buf.set_string(area.x, area.y + row as u16, s, Style::default()); + } + } +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + // crossterm types are intentionally not imported here to avoid unused warnings + use rand::prelude::*; + + fn rand_grapheme(rng: &mut rand::rngs::StdRng) -> String { + let r: u8 = rng.gen_range(0..100); + match r { + 0..=4 => "\n".to_string(), + 5..=12 => " ".to_string(), + 13..=35 => (rng.gen_range(b'a'..=b'z') as char).to_string(), + 36..=45 => (rng.gen_range(b'A'..=b'Z') as char).to_string(), + 46..=52 => (rng.gen_range(b'0'..=b'9') as char).to_string(), + 53..=65 => { + // Some emoji (wide graphemes) + let choices = ["👍", "😊", "🐍", "🚀", "🧪", "🌟"]; + choices[rng.gen_range(0..choices.len())].to_string() + } + 66..=75 => { + // CJK wide characters + let choices = ["漢", "字", "測", "試", "你", "好", "界", "编", "码"]; + choices[rng.gen_range(0..choices.len())].to_string() + } + 76..=85 => { + // Combining mark sequences + let base = ["e", "a", "o", "n", "u"][rng.gen_range(0..5)]; + let marks = ["\u{0301}", "\u{0308}", "\u{0302}", "\u{0303}"]; + format!("{}{}", base, marks[rng.gen_range(0..marks.len())]) + } + 86..=92 => { + // Some non-latin single codepoints (Greek, Cyrillic, Hebrew) + let choices = ["Ω", "β", "Ж", "ю", "ש", "م", "ह"]; + choices[rng.gen_range(0..choices.len())].to_string() + } + _ => { + // ZWJ sequences (single graphemes but multi-codepoint) + let choices = [ + "👩\u{200D}💻", // woman technologist + "👨\u{200D}💻", // man technologist + "🏳️\u{200D}🌈", // rainbow flag + ]; + choices[rng.gen_range(0..choices.len())].to_string() + } + } + } + + fn ta_with(text: &str) -> TextArea { + let mut t = TextArea::new(); + t.insert_str(text); + t + } + + #[test] + fn insert_and_replace_update_cursor_and_text() { + // insert helpers + let mut t = ta_with("hello"); + t.set_cursor(5); + t.insert_str("!"); + assert_eq!(t.text(), "hello!"); + assert_eq!(t.cursor(), 6); + + t.insert_str_at(0, "X"); + assert_eq!(t.text(), "Xhello!"); + assert_eq!(t.cursor(), 7); + + // Insert after the cursor should not move it + t.set_cursor(1); + let end = t.text().len(); + t.insert_str_at(end, "Y"); + assert_eq!(t.text(), "Xhello!Y"); + assert_eq!(t.cursor(), 1); + + // replace_range cases + // 1) cursor before range + let mut t = ta_with("abcd"); + t.set_cursor(1); + t.replace_range(2..3, "Z"); + assert_eq!(t.text(), "abZd"); + assert_eq!(t.cursor(), 1); + + // 2) cursor inside range + let mut t = ta_with("abcd"); + t.set_cursor(2); + t.replace_range(1..3, "Q"); + assert_eq!(t.text(), "aQd"); + assert_eq!(t.cursor(), 2); + + // 3) cursor after range with shifted by diff + let mut t = ta_with("abcd"); + t.set_cursor(4); + t.replace_range(0..1, "AA"); + assert_eq!(t.text(), "AAbcd"); + assert_eq!(t.cursor(), 5); + } + + #[test] + fn delete_backward_and_forward_edges() { + let mut t = ta_with("abc"); + t.set_cursor(1); + t.delete_backward(1); + assert_eq!(t.text(), "bc"); + assert_eq!(t.cursor(), 0); + + // deleting backward at start is a no-op + t.set_cursor(0); + t.delete_backward(1); + assert_eq!(t.text(), "bc"); + assert_eq!(t.cursor(), 0); + + // forward delete removes next grapheme + t.set_cursor(1); + t.delete_forward(1); + assert_eq!(t.text(), "b"); + assert_eq!(t.cursor(), 1); + + // forward delete at end is a no-op + t.set_cursor(t.text().len()); + t.delete_forward(1); + assert_eq!(t.text(), "b"); + } + + #[test] + fn delete_backward_word_and_kill_line_variants() { + // delete backward word at end removes the whole previous word + let mut t = ta_with("hello world "); + t.set_cursor(t.text().len()); + t.delete_backward_word(); + assert_eq!(t.text(), "hello "); + assert_eq!(t.cursor(), 8); + + // From inside a word, delete from word start to cursor + let mut t = ta_with("foo bar"); + t.set_cursor(6); // inside "bar" (after 'a') + t.delete_backward_word(); + assert_eq!(t.text(), "foo r"); + assert_eq!(t.cursor(), 4); + + // From end, delete the last word only + let mut t = ta_with("foo bar"); + t.set_cursor(t.text().len()); + t.delete_backward_word(); + assert_eq!(t.text(), "foo "); + assert_eq!(t.cursor(), 4); + + // kill_to_end_of_line when not at EOL + let mut t = ta_with("abc\ndef"); + t.set_cursor(1); // on first line, middle + t.kill_to_end_of_line(); + assert_eq!(t.text(), "a\ndef"); + assert_eq!(t.cursor(), 1); + + // kill_to_end_of_line when at EOL deletes newline + let mut t = ta_with("abc\ndef"); + t.set_cursor(3); // EOL of first line + t.kill_to_end_of_line(); + assert_eq!(t.text(), "abcdef"); + assert_eq!(t.cursor(), 3); + + // kill_to_beginning_of_line from middle of line + let mut t = ta_with("abc\ndef"); + t.set_cursor(5); // on second line, after 'e' + t.kill_to_beginning_of_line(); + assert_eq!(t.text(), "abc\nef"); + + // kill_to_beginning_of_line at beginning of non-first line removes the previous newline + let mut t = ta_with("abc\ndef"); + t.set_cursor(4); // beginning of second line + t.kill_to_beginning_of_line(); + assert_eq!(t.text(), "abcdef"); + assert_eq!(t.cursor(), 3); + } + + #[test] + fn cursor_left_and_right_handle_graphemes() { + let mut t = ta_with("a👍b"); + t.set_cursor(t.text().len()); + + t.move_cursor_left(); // before 'b' + let after_first_left = t.cursor(); + t.move_cursor_left(); // before '👍' + let after_second_left = t.cursor(); + t.move_cursor_left(); // before 'a' + let after_third_left = t.cursor(); + + assert!(after_first_left < t.text().len()); + assert!(after_second_left < after_first_left); + assert!(after_third_left < after_second_left); + + // Move right back to end safely + t.move_cursor_right(); + t.move_cursor_right(); + t.move_cursor_right(); + assert_eq!(t.cursor(), t.text().len()); + } + + #[test] + fn cursor_vertical_movement_across_lines_and_bounds() { + let mut t = ta_with("short\nloooooooooong\nmid"); + // Place cursor on second line, column 5 + let second_line_start = 6; // after first '\n' + t.set_cursor(second_line_start + 5); + + // Move up: target column preserved, clamped by line length + t.move_cursor_up(); + assert_eq!(t.cursor(), 5); // first line has len 5 + + // Move up again goes to start of text + t.move_cursor_up(); + assert_eq!(t.cursor(), 0); + + // Move down: from start to target col tracked + t.move_cursor_down(); + // On first move down, we should land on second line, at col 0 (target col remembered as 0) + let pos_after_down = t.cursor(); + assert!(pos_after_down >= second_line_start); + + // Move down again to third line; clamp to its length + t.move_cursor_down(); + let third_line_start = t.text().find("mid").unwrap(); + let third_line_end = third_line_start + 3; + assert!(t.cursor() >= third_line_start && t.cursor() <= third_line_end); + + // Moving down at last line jumps to end + t.move_cursor_down(); + assert_eq!(t.cursor(), t.text().len()); + } + + #[test] + fn home_end_and_emacs_style_home_end() { + let mut t = ta_with("one\ntwo\nthree"); + // Position at middle of second line + let second_line_start = t.text().find("two").unwrap(); + t.set_cursor(second_line_start + 1); + + t.move_cursor_to_beginning_of_line(false); + assert_eq!(t.cursor(), second_line_start); + + // Ctrl-A behavior: if at BOL, go to beginning of previous line + t.move_cursor_to_beginning_of_line(true); + assert_eq!(t.cursor(), 0); // beginning of first line + + // Move to EOL of first line + t.move_cursor_to_end_of_line(false); + assert_eq!(t.cursor(), 3); + + // Ctrl-E: if at EOL, go to end of next line + t.move_cursor_to_end_of_line(true); + // end of second line ("two") is right before its '\n' + let end_second_nl = t.text().find("\nthree").unwrap(); + assert_eq!(t.cursor(), end_second_nl); + } + + #[test] + fn end_of_line_or_down_at_end_of_text() { + let mut t = ta_with("one\ntwo"); + // Place cursor at absolute end of the text + t.set_cursor(t.text().len()); + // Should remain at end without panicking + t.move_cursor_to_end_of_line(true); + assert_eq!(t.cursor(), t.text().len()); + + // Also verify behavior when at EOL of a non-final line: + let eol_first_line = 3; // index of '\n' in "one\ntwo" + t.set_cursor(eol_first_line); + t.move_cursor_to_end_of_line(true); + assert_eq!(t.cursor(), t.text().len()); // moves to end of next (last) line + } + + #[test] + fn word_navigation_helpers() { + let t = ta_with(" alpha beta gamma"); + let mut t = t; // make mutable for set_cursor + // Put cursor after "alpha" + let after_alpha = t.text().find("alpha").unwrap() + "alpha".len(); + t.set_cursor(after_alpha); + assert_eq!(t.beginning_of_previous_word(), 2); // skip initial spaces + + // Put cursor at start of beta + let beta_start = t.text().find("beta").unwrap(); + t.set_cursor(beta_start); + assert_eq!(t.end_of_next_word(), beta_start + "beta".len()); + + // If at end, end_of_next_word returns len + t.set_cursor(t.text().len()); + assert_eq!(t.end_of_next_word(), t.text().len()); + } + + #[test] + fn wrapping_and_cursor_positions() { + let mut t = ta_with("hello world here"); + let area = Rect::new(0, 0, 6, 10); // width 6 -> wraps words + // desired height counts wrapped lines + assert!(t.desired_height(area.width) >= 3); + + // Place cursor in "world" + let world_start = t.text().find("world").unwrap(); + t.set_cursor(world_start + 3); + let (_x, y) = t.cursor_pos(area).unwrap(); + assert_eq!(y, 1); // world should be on second wrapped line + + // With state and small height, cursor is mapped onto visible row + let mut state = TextAreaState::default(); + let small_area = Rect::new(0, 0, 6, 1); + // First call: cursor not visible -> effective scroll ensures it is + let (_x, y) = t.cursor_pos_with_state(small_area, &state).unwrap(); + assert_eq!(y, 0); + + // Render with state to update actual scroll value + let mut buf = Buffer::empty(small_area); + ratatui::widgets::StatefulWidgetRef::render_ref(&(&t), small_area, &mut buf, &mut state); + // After render, state.scroll should be adjusted so cursor row fits + let effective_lines = t.desired_height(small_area.width); + assert!(state.scroll < effective_lines); + } + + #[test] + fn cursor_pos_with_state_basic_and_scroll_behaviors() { + // Case 1: No wrapping needed, height fits — scroll ignored, y maps directly. + let mut t = ta_with("hello world"); + t.set_cursor(3); + let area = Rect::new(2, 5, 20, 3); + // Even if an absurd scroll is provided, when content fits the area the + // effective scroll is 0 and the cursor position matches cursor_pos. + let bad_state = TextAreaState { scroll: 999 }; + let (x1, y1) = t.cursor_pos(area).unwrap(); + let (x2, y2) = t.cursor_pos_with_state(area, &bad_state).unwrap(); + assert_eq!((x2, y2), (x1, y1)); + + // Case 2: Cursor below the current window — y should be clamped to the + // bottom row (area.height - 1) after adjusting effective scroll. + let mut t = ta_with("one two three four five six"); + // Force wrapping to many visual lines. + let wrap_width = 4; + let _ = t.desired_height(wrap_width); + // Put cursor somewhere near the end so it's definitely below the first window. + t.set_cursor(t.text().len().saturating_sub(2)); + let small_area = Rect::new(0, 0, wrap_width, 2); + let state = TextAreaState { scroll: 0 }; + let (_x, y) = t.cursor_pos_with_state(small_area, &state).unwrap(); + assert_eq!(y, small_area.y + small_area.height - 1); + + // Case 3: Cursor above the current window — y should be top row (0) + // when the provided scroll is too large. + let mut t = ta_with("alpha beta gamma delta epsilon zeta"); + let wrap_width = 5; + let lines = t.desired_height(wrap_width); + // Place cursor near start so an excessive scroll moves it to top row. + t.set_cursor(1); + let area = Rect::new(0, 0, wrap_width, 3); + let state = TextAreaState { + scroll: lines.saturating_mul(2), + }; + let (_x, y) = t.cursor_pos_with_state(area, &state).unwrap(); + assert_eq!(y, area.y); + } + + #[test] + fn wrapped_navigation_across_visual_lines() { + let mut t = ta_with("abcdefghij"); + // Force wrapping at width 4: lines -> ["abcd", "efgh", "ij"] + let _ = t.desired_height(4); + + // From the very start, moving down should go to the start of the next wrapped line (index 4) + t.set_cursor(0); + t.move_cursor_down(); + assert_eq!(t.cursor(), 4); + + // Cursor at boundary index 4 should be displayed at start of second wrapped line + t.set_cursor(4); + let area = Rect::new(0, 0, 4, 10); + let (x, y) = t.cursor_pos(area).unwrap(); + assert_eq!((x, y), (0, 1)); + + // With state and small height, cursor should be visible at row 0, col 0 + let small_area = Rect::new(0, 0, 4, 1); + let state = TextAreaState::default(); + let (x, y) = t.cursor_pos_with_state(small_area, &state).unwrap(); + assert_eq!((x, y), (0, 0)); + + // Place cursor in the middle of the second wrapped line ("efgh"), at 'g' + t.set_cursor(6); + // Move up should go to same column on previous wrapped line -> index 2 ('c') + t.move_cursor_up(); + assert_eq!(t.cursor(), 2); + + // Move down should return to same position on the next wrapped line -> back to index 6 ('g') + t.move_cursor_down(); + assert_eq!(t.cursor(), 6); + + // Move down again should go to third wrapped line. Target col is 2, but the line has len 2 -> clamp to end + t.move_cursor_down(); + assert_eq!(t.cursor(), t.text().len()); + } + + #[test] + fn cursor_pos_with_state_after_movements() { + let mut t = ta_with("abcdefghij"); + // Wrap width 4 -> visual lines: abcd | efgh | ij + let _ = t.desired_height(4); + let area = Rect::new(0, 0, 4, 2); + let mut state = TextAreaState::default(); + let mut buf = Buffer::empty(area); + + // Start at beginning + t.set_cursor(0); + ratatui::widgets::StatefulWidgetRef::render_ref(&(&t), area, &mut buf, &mut state); + let (x, y) = t.cursor_pos_with_state(area, &state).unwrap(); + assert_eq!((x, y), (0, 0)); + + // Move down to second visual line; should be at bottom row (row 1) within 2-line viewport + t.move_cursor_down(); + ratatui::widgets::StatefulWidgetRef::render_ref(&(&t), area, &mut buf, &mut state); + let (x, y) = t.cursor_pos_with_state(area, &state).unwrap(); + assert_eq!((x, y), (0, 1)); + + // Move down to third visual line; viewport scrolls and keeps cursor on bottom row + t.move_cursor_down(); + ratatui::widgets::StatefulWidgetRef::render_ref(&(&t), area, &mut buf, &mut state); + let (x, y) = t.cursor_pos_with_state(area, &state).unwrap(); + assert_eq!((x, y), (0, 1)); + + // Move up to second visual line; with current scroll, it appears on top row + t.move_cursor_up(); + ratatui::widgets::StatefulWidgetRef::render_ref(&(&t), area, &mut buf, &mut state); + let (x, y) = t.cursor_pos_with_state(area, &state).unwrap(); + assert_eq!((x, y), (0, 0)); + + // Column preservation across moves: set to col 2 on first line, move down + t.set_cursor(2); + ratatui::widgets::StatefulWidgetRef::render_ref(&(&t), area, &mut buf, &mut state); + let (x0, y0) = t.cursor_pos_with_state(area, &state).unwrap(); + assert_eq!((x0, y0), (2, 0)); + t.move_cursor_down(); + ratatui::widgets::StatefulWidgetRef::render_ref(&(&t), area, &mut buf, &mut state); + let (x1, y1) = t.cursor_pos_with_state(area, &state).unwrap(); + assert_eq!((x1, y1), (2, 1)); + } + + #[test] + fn wrapped_navigation_with_newlines_and_spaces() { + // Include spaces and an explicit newline to exercise boundaries + let mut t = ta_with("word1 word2\nword3"); + // Width 6 will wrap "word1 " and then "word2" before the newline + let _ = t.desired_height(6); + + // Put cursor on the second wrapped line before the newline, at column 1 of "word2" + let start_word2 = t.text().find("word2").unwrap(); + t.set_cursor(start_word2 + 1); + + // Up should go to first wrapped line, column 1 -> index 1 + t.move_cursor_up(); + assert_eq!(t.cursor(), 1); + + // Down should return to the same visual column on "word2" + t.move_cursor_down(); + assert_eq!(t.cursor(), start_word2 + 1); + + // Down again should cross the logical newline to the next visual line ("word3"), clamped to its length if needed + t.move_cursor_down(); + let start_word3 = t.text().find("word3").unwrap(); + assert!(t.cursor() >= start_word3 && t.cursor() <= start_word3 + "word3".len()); + } + + #[test] + fn wrapped_navigation_with_wide_graphemes() { + // Four thumbs up, each of display width 2, with width 3 to force wrapping inside grapheme boundaries + let mut t = ta_with("👍👍👍👍"); + let _ = t.desired_height(3); + + // Put cursor after the second emoji (which should be on first wrapped line) + t.set_cursor("👍👍".len()); + + // Move down should go to the start of the next wrapped line (same column preserved but clamped) + t.move_cursor_down(); + // We expect to land somewhere within the third emoji or at the start of it + let pos_after_down = t.cursor(); + assert!(pos_after_down >= "👍👍".len()); + + // Moving up should take us back to the original position + t.move_cursor_up(); + assert_eq!(t.cursor(), "👍👍".len()); + } + + #[test] + fn fuzz_textarea_randomized() { + // Deterministic seed for reproducibility + // Seed the RNG based on the current day in Pacific Time (PST/PDT). This + // keeps the fuzz test deterministic within a day while still varying + // day-to-day to improve coverage. + #[allow(clippy::unwrap_used)] + let pst_today_seed: u64 = (chrono::Utc::now() - chrono::Duration::hours(8)) + .date_naive() + .and_hms_opt(0, 0, 0) + .unwrap() + .and_utc() + .timestamp() as u64; + let mut rng = rand::rngs::StdRng::seed_from_u64(pst_today_seed); + + for _case in 0..10_000 { + let mut ta = TextArea::new(); + let mut state = TextAreaState::default(); + // Start with a random base string + let base_len = rng.gen_range(0..30); + let mut base = String::new(); + for _ in 0..base_len { + base.push_str(&rand_grapheme(&mut rng)); + } + ta.set_text(&base); + // Choose a valid char boundary for initial cursor + let mut boundaries: Vec = vec![0]; + boundaries.extend(ta.text().char_indices().map(|(i, _)| i).skip(1)); + boundaries.push(ta.text().len()); + let init = boundaries[rng.gen_range(0..boundaries.len())]; + ta.set_cursor(init); + + let mut width: u16 = rng.gen_range(1..=12); + let mut height: u16 = rng.gen_range(1..=4); + + for _step in 0..200 { + // Mostly stable width/height, occasionally change + if rng.gen_bool(0.1) { + width = rng.gen_range(1..=12); + } + if rng.gen_bool(0.1) { + height = rng.gen_range(1..=4); + } + + // Pick an operation + match rng.gen_range(0..14) { + 0 => { + // insert small random string at cursor + let len = rng.gen_range(0..6); + let mut s = String::new(); + for _ in 0..len { + s.push_str(&rand_grapheme(&mut rng)); + } + ta.insert_str(&s); + } + 1 => { + // replace_range with small random slice + let mut b: Vec = vec![0]; + b.extend(ta.text().char_indices().map(|(i, _)| i).skip(1)); + b.push(ta.text().len()); + let i1 = rng.gen_range(0..b.len()); + let i2 = rng.gen_range(0..b.len()); + let (start, end) = if b[i1] <= b[i2] { + (b[i1], b[i2]) + } else { + (b[i2], b[i1]) + }; + let insert_len = rng.gen_range(0..=4); + let mut s = String::new(); + for _ in 0..insert_len { + s.push_str(&rand_grapheme(&mut rng)); + } + let before = ta.text().len(); + ta.replace_range(start..end, &s); + let after = ta.text().len(); + assert_eq!( + after as isize, + before as isize + (s.len() as isize) - ((end - start) as isize) + ); + } + 2 => ta.delete_backward(rng.gen_range(0..=3)), + 3 => ta.delete_forward(rng.gen_range(0..=3)), + 4 => ta.delete_backward_word(), + 5 => ta.kill_to_beginning_of_line(), + 6 => ta.kill_to_end_of_line(), + 7 => ta.move_cursor_left(), + 8 => ta.move_cursor_right(), + 9 => ta.move_cursor_up(), + 10 => ta.move_cursor_down(), + 11 => ta.move_cursor_to_beginning_of_line(true), + 12 => ta.move_cursor_to_end_of_line(true), + _ => { + // Jump to word boundaries + if rng.gen_bool(0.5) { + let p = ta.beginning_of_previous_word(); + ta.set_cursor(p); + } else { + let p = ta.end_of_next_word(); + ta.set_cursor(p); + } + } + } + + // Sanity invariants + assert!(ta.cursor() <= ta.text().len()); + + // Render and compute cursor positions; ensure they are in-bounds and do not panic + let area = Rect::new(0, 0, width, height); + // Stateless render into an area tall enough for all wrapped lines + let total_lines = ta.desired_height(width); + let full_area = Rect::new(0, 0, width, total_lines.max(1)); + let mut buf = Buffer::empty(full_area); + ratatui::widgets::WidgetRef::render_ref(&(&ta), full_area, &mut buf); + + // cursor_pos: x must be within width when present + let _ = ta.cursor_pos(area); + + // cursor_pos_with_state: always within viewport rows + let (_x, _y) = ta + .cursor_pos_with_state(area, &state) + .unwrap_or((area.x, area.y)); + + // Stateful render should not panic, and updates scroll + let mut sbuf = Buffer::empty(area); + ratatui::widgets::StatefulWidgetRef::render_ref( + &(&ta), + area, + &mut sbuf, + &mut state, + ); + + // After wrapping, desired height equals the number of lines we would render without scroll + let total_lines = total_lines as usize; + // state.scroll must not exceed total_lines when content fits within area height + if (height as usize) >= total_lines { + assert_eq!(state.scroll, 0); + } + } + } + } +} diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 374a4b5688..31151d5646 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -509,6 +509,10 @@ impl ChatWidget<'_> { self.bottom_pane .set_token_usage(self.token_usage.clone(), self.config.model_context_window); } + + pub fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { + self.bottom_pane.cursor_pos(area) + } } impl WidgetRef for &ChatWidget<'_> { From 78a1d49fac0fbc150e5e862fd3f24c8af50e393c Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Sun, 3 Aug 2025 11:33:44 -0700 Subject: [PATCH 08/18] fix command duration display (#1806) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit we were always displaying "0ms" before. Screenshot 2025-08-02 at 10 51 22 PM --- codex-rs/core/src/codex.rs | 50 +++++++++---------- codex-rs/core/src/protocol.rs | 2 + codex-rs/core/tests/live_agent.rs | 6 +-- .../src/event_processor_with_human_output.rs | 10 ++-- codex-rs/tui/src/chatwidget.rs | 4 +- 5 files changed, 32 insertions(+), 40 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index e759acb4a2..7004dcfcb7 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -396,11 +396,15 @@ impl Session { &self, sub_id: &str, call_id: &str, - stdout: &str, - stderr: &str, - exit_code: i32, + output: &ExecToolCallOutput, is_apply_patch: bool, ) { + let ExecToolCallOutput { + stdout, + stderr, + duration, + exit_code, + } = output; // Because stdout and stderr could each be up to 100 KiB, we send // truncated versions. const MAX_STREAM_OUTPUT: usize = 5 * 1024; // 5KiB @@ -412,14 +416,15 @@ impl Session { call_id: call_id.to_string(), stdout, stderr, - success: exit_code == 0, + success: *exit_code == 0, }) } else { EventMsg::ExecCommandEnd(ExecCommandEndEvent { call_id: call_id.to_string(), stdout, stderr, - exit_code, + duration: *duration, + exit_code: *exit_code, }) }; @@ -1775,23 +1780,21 @@ async fn handle_container_exec_with_params( stdout, stderr, duration, - } = output; + } = &output; sess.notify_exec_command_end( &sub_id, &call_id, - &stdout, - &stderr, - exit_code, + &output, exec_command_context.apply_patch.is_some(), ) .await; - let is_success = exit_code == 0; + let is_success = *exit_code == 0; let content = format_exec_output( - if is_success { &stdout } else { &stderr }, - exit_code, - duration, + if is_success { stdout } else { stderr }, + *exit_code, + *duration, ); ResponseInputItem::FunctionCallOutput { @@ -1900,23 +1903,16 @@ async fn handle_sandbox_error( stdout, stderr, duration, - } = retry_output; + } = &retry_output; - sess.notify_exec_command_end( - &sub_id, - &call_id, - &stdout, - &stderr, - exit_code, - is_apply_patch, - ) - .await; + sess.notify_exec_command_end(&sub_id, &call_id, &retry_output, is_apply_patch) + .await; - let is_success = exit_code == 0; + let is_success = *exit_code == 0; let content = format_exec_output( - if is_success { &stdout } else { &stderr }, - exit_code, - duration, + if is_success { stdout } else { stderr }, + *exit_code, + *duration, ); ResponseInputItem::FunctionCallOutput { diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index 1bfeee56ab..cbb211d955 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -523,6 +523,8 @@ pub struct ExecCommandEndEvent { pub stderr: String, /// The command's exit code. pub exit_code: i32, + /// The duration of the command execution. + pub duration: Duration, } #[derive(Debug, Clone, Deserialize, Serialize)] diff --git a/codex-rs/core/tests/live_agent.rs b/codex-rs/core/tests/live_agent.rs index 95408e20e5..81b3bb2a12 100644 --- a/codex-rs/core/tests/live_agent.rs +++ b/codex-rs/core/tests/live_agent.rs @@ -177,8 +177,7 @@ async fn live_shell_function_call() { match ev.msg { EventMsg::ExecCommandBegin(codex_core::protocol::ExecCommandBeginEvent { command, - call_id: _, - cwd: _, + .. }) => { assert_eq!(command, vec!["echo", MARKER]); saw_begin = true; @@ -186,8 +185,7 @@ async fn live_shell_function_call() { EventMsg::ExecCommandEnd(codex_core::protocol::ExecCommandEndEvent { stdout, exit_code, - call_id: _, - stderr: _, + .. }) => { assert_eq!(exit_code, 0, "echo returned non‑zero exit code"); assert!(stdout.contains(MARKER)); diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 7393ab72a0..72e2f9298f 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -106,7 +106,6 @@ impl EventProcessorWithHumanOutput { struct ExecCommandBegin { command: Vec, - start_time: Instant, } struct PatchApplyBegin { @@ -228,7 +227,6 @@ impl EventProcessor for EventProcessorWithHumanOutput { call_id.clone(), ExecCommandBegin { command: command.clone(), - start_time: Instant::now(), }, ); ts_println!( @@ -244,16 +242,14 @@ impl EventProcessor for EventProcessorWithHumanOutput { call_id, stdout, stderr, + duration, exit_code, }) => { let exec_command = self.call_id_to_command.remove(&call_id); - let (duration, call) = if let Some(ExecCommandBegin { - command, - start_time, - }) = exec_command + let (duration, call) = if let Some(ExecCommandBegin { command, .. }) = exec_command { ( - format!(" in {}", format_elapsed(start_time)), + format!(" in {}", format_duration(duration)), format!("{}", escape_command(&command).style(self.bold)), ) } else { diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 31151d5646..e5ebf58a07 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1,7 +1,6 @@ use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; -use std::time::Duration; use codex_core::codex_wrapper::CodexConversation; use codex_core::codex_wrapper::init_codex; @@ -390,6 +389,7 @@ impl ChatWidget<'_> { EventMsg::ExecCommandEnd(ExecCommandEndEvent { call_id, exit_code, + duration, stdout, stderr, }) => { @@ -400,7 +400,7 @@ impl ChatWidget<'_> { exit_code, stdout, stderr, - duration: Duration::from_secs(0), + duration, }, )); } From 2576fadc742cc0030800214be55c5c7833521679 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Sun, 3 Aug 2025 11:51:33 -0700 Subject: [PATCH 09/18] shimmer on working (#1807) change the animation on "working" to be a text shimmer https://github.com/user-attachments/assets/f64529eb-1c64-493a-8d97-0f68b964bdd0 --- codex-rs/Cargo.lock | 16 +++++ codex-rs/tui/Cargo.toml | 1 + codex-rs/tui/src/status_indicator_widget.rs | 73 +++++++++++++-------- 3 files changed, 61 insertions(+), 29 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index e1a0e162dc..7d4e41d0b1 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -869,6 +869,7 @@ dependencies = [ "shlex", "strum 0.27.2", "strum_macros 0.27.2", + "supports-color", "textwrap 0.16.2", "tokio", "tracing", @@ -2337,6 +2338,12 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "is_ci" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7655c9839580ee829dfacba1d1278c2b7883e50a277ff7541299489d6bdfdc45" + [[package]] name = "is_terminal_polyfill" version = "1.70.1" @@ -4378,6 +4385,15 @@ version = "2.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" +[[package]] +name = "supports-color" +version = "3.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c64fc7232dd8d2e4ac5ce4ef302b1d81e0b80d055b9d77c7c4f51f6aa4c867d6" +dependencies = [ + "is_ci", +] + [[package]] name = "syn" version = "1.0.109" diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 823fd1428e..a571b32c8d 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -48,6 +48,7 @@ serde_json = { version = "1", features = ["preserve_order"] } shlex = "1.3.0" strum = "0.27.2" strum_macros = "0.27.2" +supports-color = "3.0.2" textwrap = "0.16.2" tokio = { version = "1", features = [ "io-std", diff --git a/codex-rs/tui/src/status_indicator_widget.rs b/codex-rs/tui/src/status_indicator_widget.rs index 7e6d267481..aa18ac6fa5 100644 --- a/codex-rs/tui/src/status_indicator_widget.rs +++ b/codex-rs/tui/src/status_indicator_widget.rs @@ -57,7 +57,7 @@ impl StatusIndicatorWidget { thread::spawn(move || { let mut counter = 0usize; while running_clone.load(Ordering::Relaxed) { - std::thread::sleep(Duration::from_millis(200)); + std::thread::sleep(Duration::from_millis(100)); counter = counter.wrapping_add(1); frame_idx_clone.store(counter, Ordering::Relaxed); app_event_tx_clone.send(AppEvent::RequestRedraw); @@ -98,46 +98,51 @@ impl WidgetRef for StatusIndicatorWidget { .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. - const DOT_COUNT: usize = 3; let idx = self.frame_idx.load(std::sync::atomic::Ordering::Relaxed); - let phase = idx % (DOT_COUNT * 2 - 2); - let active = if phase < DOT_COUNT { - phase - } else { - (DOT_COUNT * 2 - 2) - phase - }; + let header_text = "Working"; + let header_chars: Vec = header_text.chars().collect(); + + let padding = 4usize; // virtual padding around the word for smoother loop + let period = header_chars.len() + padding * 2; + let pos = idx % period; + + let has_true_color = supports_color::on_cached(supports_color::Stream::Stdout) + .map(|level| level.has_16m) + .unwrap_or(false); + + // Width of the bright band (in characters). + let band_half_width = 2.0; let mut header_spans: Vec> = Vec::new(); + for (i, ch) in header_chars.iter().enumerate() { + let i_pos = i as isize + padding as isize; + let pos = pos as isize; + let dist = (i_pos - pos).abs() as f32; - header_spans.push(Span::styled( - "Working ", - Style::default() - .fg(Color::White) - .add_modifier(Modifier::BOLD), - )); + let t = if dist <= band_half_width { + let x = std::f32::consts::PI * (dist / band_half_width); + 0.5 * (1.0 + x.cos()) + } else { + 0.0 + }; - header_spans.push(Span::styled( - "[", - Style::default() - .fg(Color::White) - .add_modifier(Modifier::BOLD), - )); - - for i in 0..DOT_COUNT { - let style = if i == active { + let brightness = 0.4 + 0.6 * t; + let level = (brightness * 255.0).clamp(0.0, 255.0) as u8; + let style = if has_true_color { Style::default() - .fg(Color::White) + .fg(Color::Rgb(level, level, level)) .add_modifier(Modifier::BOLD) } else { - Style::default().dim() + // Bold makes dark gray and gray look the same, so don't use it + // when true color is not supported. + Style::default().fg(color_for_level(level)) }; - header_spans.push(Span::styled(".", style)); + + header_spans.push(Span::styled(ch.to_string(), style)); } header_spans.push(Span::styled( - "] ", + " ", Style::default() .fg(Color::White) .add_modifier(Modifier::BOLD), @@ -189,3 +194,13 @@ impl WidgetRef for StatusIndicatorWidget { paragraph.render_ref(area, buf); } } + +fn color_for_level(level: u8) -> Color { + if level < 128 { + Color::DarkGray + } else if level < 192 { + Color::Gray + } else { + Color::White + } +} From e3565a3f438c30c9d36412d2817346c7accd487c Mon Sep 17 00:00:00 2001 From: Dylan Date: Sun, 3 Aug 2025 13:05:48 -0700 Subject: [PATCH 10/18] [sandbox] Filter out certain non-sandbox errors (#1804) ## Summary Users frequently complain about re-approving commands that have failed for non-sandbox reasons. We can't diagnose with complete accuracy which errors happened because of a sandbox failure, but we can start to eliminate some common simple cases. This PR captures the most common case I've seen, which is a `command not found` error. ## Testing - [x] Added unit tests - [x] Ran a few cases locally --- codex-rs/core/src/exec.rs | 26 +++++++++++--- codex-rs/core/src/lib.rs | 3 +- codex-rs/core/tests/exec.rs | 69 +++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 codex-rs/core/tests/exec.rs diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 5301f0220d..dce02cc5e2 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -140,11 +140,7 @@ pub async fn process_exec_tool_call( let exit_code = raw_output.exit_status.code().unwrap_or(-1); - // NOTE(ragona): This is much less restrictive than the previous check. If we exec - // a command, and it returns anything other than success, we assume that it may have - // been a sandboxing error and allow the user to retry. (The user of course may choose - // not to retry, or in a non-interactive mode, would automatically reject the approval.) - if exit_code != 0 && sandbox_type != SandboxType::None { + if exit_code != 0 && is_likely_sandbox_denied(sandbox_type, exit_code) { return Err(CodexErr::Sandbox(SandboxErr::Denied( exit_code, stdout, stderr, ))); @@ -223,6 +219,26 @@ fn create_linux_sandbox_command_args( linux_cmd } +/// We don't have a fully deterministic way to tell if our command failed +/// because of the sandbox - a command in the user's zshrc file might hit an +/// error, but the command itself might fail or succeed for other reasons. +/// For now, we conservatively check for 'command not found' (exit code 127), +/// and can add additional cases as necessary. +fn is_likely_sandbox_denied(sandbox_type: SandboxType, exit_code: i32) -> bool { + if sandbox_type == SandboxType::None { + return false; + } + + // Quick rejects: well-known non-sandbox shell exit codes + // 127: command not found, 2: misuse of shell builtins + if exit_code == 127 { + return false; + } + + // For all other cases, we assume the sandbox is the cause + true +} + #[derive(Debug)] pub struct RawExecToolCallOutput { pub exit_status: ExitStatus, diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index a33e185afb..80f9014954 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -38,7 +38,7 @@ pub mod plan_tool; mod project_doc; pub mod protocol; mod rollout; -mod safety; +pub(crate) mod safety; pub mod seatbelt; pub mod shell; pub mod spawn; @@ -47,3 +47,4 @@ pub mod util; pub use apply_patch::CODEX_APPLY_PATCH_ARG1; pub use client_common::model_supports_reasoning_summaries; +pub use safety::get_platform_sandbox; diff --git a/codex-rs/core/tests/exec.rs b/codex-rs/core/tests/exec.rs new file mode 100644 index 0000000000..da169296ed --- /dev/null +++ b/codex-rs/core/tests/exec.rs @@ -0,0 +1,69 @@ +#![cfg(target_os = "macos")] +#![expect(clippy::expect_used)] + +use std::collections::HashMap; +use std::sync::Arc; + +use codex_core::exec::ExecParams; +use codex_core::exec::SandboxType; +use codex_core::exec::process_exec_tool_call; +use codex_core::protocol::SandboxPolicy; +use codex_core::spawn::CODEX_SANDBOX_ENV_VAR; +use tempfile::TempDir; +use tokio::sync::Notify; + +use codex_core::get_platform_sandbox; + +async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>, should_be_ok: bool) { + if std::env::var(CODEX_SANDBOX_ENV_VAR) == Ok("seatbelt".to_string()) { + eprintln!("{CODEX_SANDBOX_ENV_VAR} is set to 'seatbelt', skipping test."); + return; + } + + let sandbox_type = get_platform_sandbox().expect("should be able to get sandbox type"); + assert_eq!(sandbox_type, SandboxType::MacosSeatbelt); + + let params = ExecParams { + command: cmd.iter().map(|s| s.to_string()).collect(), + cwd: tmp.path().to_path_buf(), + timeout_ms: Some(1000), + env: HashMap::new(), + }; + + let ctrl_c = Arc::new(Notify::new()); + let policy = SandboxPolicy::new_read_only_policy(); + + let result = process_exec_tool_call(params, sandbox_type, ctrl_c, &policy, &None, None).await; + + assert!(result.is_ok() == should_be_ok); +} + +/// Command succeeds with exit code 0 normally +#[tokio::test] +async fn exit_code_0_succeeds() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let cmd = vec!["echo", "hello"]; + + run_test_cmd(tmp, cmd, true).await +} + +/// Command not found returns exit code 127, this is not considered a sandbox error +#[tokio::test] +async fn exit_command_not_found_is_ok() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let cmd = vec!["/bin/bash", "-c", "nonexistent_command_12345"]; + run_test_cmd(tmp, cmd, true).await +} + +/// Writing a file fails and should be considered a sandbox error +#[tokio::test] +async fn write_file_fails_as_sandbox_error() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let path = tmp.path().join("test.txt"); + let cmd = vec![ + "/user/bin/touch", + path.to_str().expect("should be able to get path"), + ]; + + run_test_cmd(tmp, cmd, false).await; +} From 1f3318c1c5b83e29b6d6e61eb2bf7d599581023e Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Mon, 4 Aug 2025 08:57:04 -0700 Subject: [PATCH 11/18] Add a TurnDiffTracker to create a unified diff for an entire turn (#1770) This lets us show an accumulating diff across all patches in a turn. Refer to the docs for TurnDiffTracker for implementation details. There are multiple ways this could have been done and this felt like the right tradeoff between reliability and completeness: *Pros* * It will pick up all changes to files that the model touched including if they prettier or another command that updates them. * It will not pick up changes made by the user or other agents to files it didn't modify. *Cons* * It will pick up changes that the user made to a file that the model also touched * It will not pick up changes to codegen or files that were not modified with apply_patch --- codex-rs/Cargo.lock | 1 + codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/codex.rs | 111 ++- codex-rs/core/src/lib.rs | 1 + codex-rs/core/src/protocol.rs | 7 + codex-rs/core/src/turn_diff_tracker.rs | 887 ++++++++++++++++++ .../src/event_processor_with_human_output.rs | 6 + codex-rs/mcp-server/src/codex_tool_runner.rs | 1 + codex-rs/mcp-server/src/conversation_loop.rs | 1 + 9 files changed, 998 insertions(+), 18 deletions(-) create mode 100644 codex-rs/core/src/turn_diff_tracker.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 7d4e41d0b1..eb4eccd897 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -699,6 +699,7 @@ dependencies = [ "serde_json", "sha1", "shlex", + "similar", "strum_macros 0.27.2", "tempfile", "thiserror 2.0.12", diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index db3fd4f834..466e9adf02 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -34,6 +34,7 @@ serde_json = "1" serde_bytes = "0.11" sha1 = "0.10.6" shlex = "1.3.0" +similar = "2.7.0" strum_macros = "0.27.2" thiserror = "2.0.12" time = { version = "0.3", features = ["formatting", "local-offset", "macros"] } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 7004dcfcb7..568d87c4a8 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -85,11 +85,13 @@ use crate::protocol::SandboxPolicy; use crate::protocol::SessionConfiguredEvent; use crate::protocol::Submission; use crate::protocol::TaskCompleteEvent; +use crate::protocol::TurnDiffEvent; use crate::rollout::RolloutRecorder; use crate::safety::SafetyCheck; use crate::safety::assess_command_safety; use crate::safety::assess_safety_for_untrusted_command; use crate::shell; +use crate::turn_diff_tracker::TurnDiffTracker; use crate::user_notification::UserNotification; use crate::util::backoff; @@ -362,7 +364,11 @@ impl Session { } } - async fn notify_exec_command_begin(&self, exec_command_context: ExecCommandContext) { + async fn on_exec_command_begin( + &self, + turn_diff_tracker: &mut TurnDiffTracker, + exec_command_context: ExecCommandContext, + ) { let ExecCommandContext { sub_id, call_id, @@ -374,11 +380,15 @@ impl Session { Some(ApplyPatchCommandContext { user_explicitly_approved_this_action, changes, - }) => EventMsg::PatchApplyBegin(PatchApplyBeginEvent { - call_id, - auto_approved: !user_explicitly_approved_this_action, - changes, - }), + }) => { + turn_diff_tracker.on_patch_begin(&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(), @@ -392,8 +402,10 @@ impl Session { let _ = self.tx_event.send(event).await; } - async fn notify_exec_command_end( + #[allow(clippy::too_many_arguments)] + async fn on_exec_command_end( &self, + turn_diff_tracker: &mut TurnDiffTracker, sub_id: &str, call_id: &str, output: &ExecToolCallOutput, @@ -433,6 +445,20 @@ impl Session { msg, }; let _ = self.tx_event.send(event).await; + + // If this is an apply_patch, after we emit the end patch, emit a second event + // with the full turn diff if there is one. + if is_apply_patch { + let unified_diff = turn_diff_tracker.get_unified_diff(); + if let Ok(Some(unified_diff)) = unified_diff { + let msg = EventMsg::TurnDiff(TurnDiffEvent { unified_diff }); + let event = Event { + id: sub_id.into(), + msg, + }; + let _ = self.tx_event.send(event).await; + } + } } /// Helper that emits a BackgroundEvent with the given message. This keeps @@ -1006,6 +1032,10 @@ async fn run_task(sess: Arc, sub_id: String, input: Vec) { .await; let last_agent_message: Option; + // Although from the perspective of codex.rs, TurnDiffTracker has the lifecycle of a Task which contains + // many turns, from the perspective of the user, it is a single turn. + let mut turn_diff_tracker = TurnDiffTracker::new(); + loop { // Note that pending_input would be something like a message the user // submitted through the UI while the model was running. Though the UI @@ -1037,7 +1067,7 @@ async fn run_task(sess: Arc, sub_id: String, input: Vec) { }) }) .collect(); - match run_turn(&sess, sub_id.clone(), turn_input).await { + match run_turn(&sess, &mut turn_diff_tracker, sub_id.clone(), turn_input).await { Ok(turn_output) => { let mut items_to_record_in_conversation_history = Vec::::new(); let mut responses = Vec::::new(); @@ -1163,6 +1193,7 @@ async fn run_task(sess: Arc, sub_id: String, input: Vec) { async fn run_turn( sess: &Session, + turn_diff_tracker: &mut TurnDiffTracker, sub_id: String, input: Vec, ) -> CodexResult> { @@ -1177,7 +1208,7 @@ async fn run_turn( let mut retries = 0; loop { - match try_run_turn(sess, &sub_id, &prompt).await { + match try_run_turn(sess, turn_diff_tracker, &sub_id, &prompt).await { Ok(output) => return Ok(output), Err(CodexErr::Interrupted) => return Err(CodexErr::Interrupted), Err(CodexErr::EnvVar(var)) => return Err(CodexErr::EnvVar(var)), @@ -1223,6 +1254,7 @@ struct ProcessedResponseItem { async fn try_run_turn( sess: &Session, + turn_diff_tracker: &mut TurnDiffTracker, sub_id: &str, prompt: &Prompt, ) -> CodexResult> { @@ -1310,7 +1342,8 @@ async fn try_run_turn( match event { ResponseEvent::Created => {} ResponseEvent::OutputItemDone(item) => { - let response = handle_response_item(sess, sub_id, item.clone()).await?; + let response = + handle_response_item(sess, turn_diff_tracker, sub_id, item.clone()).await?; output.push(ProcessedResponseItem { item, response }); } @@ -1328,6 +1361,16 @@ async fn try_run_turn( .ok(); } + let unified_diff = turn_diff_tracker.get_unified_diff(); + if let Ok(Some(unified_diff)) = unified_diff { + let msg = EventMsg::TurnDiff(TurnDiffEvent { unified_diff }); + let event = Event { + id: sub_id.to_string(), + msg, + }; + let _ = sess.tx_event.send(event).await; + } + return Ok(output); } ResponseEvent::OutputTextDelta(delta) => { @@ -1432,6 +1475,7 @@ async fn run_compact_task( async fn handle_response_item( sess: &Session, + turn_diff_tracker: &mut TurnDiffTracker, sub_id: &str, item: ResponseItem, ) -> CodexResult> { @@ -1469,7 +1513,17 @@ async fn handle_response_item( .. } => { info!("FunctionCall: {arguments}"); - Some(handle_function_call(sess, sub_id.to_string(), name, arguments, call_id).await) + Some( + handle_function_call( + sess, + turn_diff_tracker, + sub_id.to_string(), + name, + arguments, + call_id, + ) + .await, + ) } ResponseItem::LocalShellCall { id, @@ -1504,6 +1558,7 @@ async fn handle_response_item( handle_container_exec_with_params( exec_params, sess, + turn_diff_tracker, sub_id.to_string(), effective_call_id, ) @@ -1521,6 +1576,7 @@ async fn handle_response_item( async fn handle_function_call( sess: &Session, + turn_diff_tracker: &mut TurnDiffTracker, sub_id: String, name: String, arguments: String, @@ -1534,7 +1590,8 @@ async fn handle_function_call( return *output; } }; - handle_container_exec_with_params(params, sess, sub_id, call_id).await + handle_container_exec_with_params(params, sess, turn_diff_tracker, sub_id, call_id) + .await } "update_plan" => handle_update_plan(sess, arguments, sub_id, call_id).await, _ => { @@ -1608,6 +1665,7 @@ fn maybe_run_with_user_profile(params: ExecParams, sess: &Session) -> ExecParams async fn handle_container_exec_with_params( params: ExecParams, sess: &Session, + turn_diff_tracker: &mut TurnDiffTracker, sub_id: String, call_id: String, ) -> ResponseInputItem { @@ -1755,7 +1813,7 @@ async fn handle_container_exec_with_params( }, ), }; - sess.notify_exec_command_begin(exec_command_context.clone()) + sess.on_exec_command_begin(turn_diff_tracker, exec_command_context.clone()) .await; let params = maybe_run_with_user_profile(params, sess); @@ -1782,7 +1840,8 @@ async fn handle_container_exec_with_params( duration, } = &output; - sess.notify_exec_command_end( + sess.on_exec_command_end( + turn_diff_tracker, &sub_id, &call_id, &output, @@ -1806,7 +1865,15 @@ async fn handle_container_exec_with_params( } } Err(CodexErr::Sandbox(error)) => { - handle_sandbox_error(params, exec_command_context, error, sandbox_type, sess).await + handle_sandbox_error( + turn_diff_tracker, + params, + exec_command_context, + error, + sandbox_type, + sess, + ) + .await } Err(e) => { // Handle non-sandbox errors @@ -1822,6 +1889,7 @@ async fn handle_container_exec_with_params( } async fn handle_sandbox_error( + turn_diff_tracker: &mut TurnDiffTracker, params: ExecParams, exec_command_context: ExecCommandContext, error: SandboxErr, @@ -1878,7 +1946,8 @@ async fn handle_sandbox_error( sess.notify_background_event(&sub_id, "retrying command without sandbox") .await; - sess.notify_exec_command_begin(exec_command_context).await; + sess.on_exec_command_begin(turn_diff_tracker, exec_command_context) + .await; // This is an escalated retry; the policy will not be // examined and the sandbox has been set to `None`. @@ -1905,8 +1974,14 @@ async fn handle_sandbox_error( duration, } = &retry_output; - sess.notify_exec_command_end(&sub_id, &call_id, &retry_output, is_apply_patch) - .await; + sess.on_exec_command_end( + turn_diff_tracker, + &sub_id, + &call_id, + &retry_output, + is_apply_patch, + ) + .await; let is_success = *exit_code == 0; let content = format_exec_output( diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 80f9014954..4f083d9e56 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -42,6 +42,7 @@ pub(crate) mod safety; pub mod seatbelt; pub mod shell; pub mod spawn; +pub mod turn_diff_tracker; mod user_notification; pub mod util; diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index cbb211d955..82591a2c78 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -387,6 +387,8 @@ pub enum EventMsg { /// Notification that a patch application has finished. PatchApplyEnd(PatchApplyEndEvent), + TurnDiff(TurnDiffEvent), + /// Response to GetHistoryEntryRequest. GetHistoryEntryResponse(GetHistoryEntryResponseEvent), @@ -598,6 +600,11 @@ pub struct PatchApplyEndEvent { pub success: bool, } +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct TurnDiffEvent { + pub unified_diff: String, +} + #[derive(Debug, Clone, Deserialize, Serialize)] pub struct GetHistoryEntryResponseEvent { pub offset: usize, diff --git a/codex-rs/core/src/turn_diff_tracker.rs b/codex-rs/core/src/turn_diff_tracker.rs new file mode 100644 index 0000000000..7026d7bb32 --- /dev/null +++ b/codex-rs/core/src/turn_diff_tracker.rs @@ -0,0 +1,887 @@ +use std::collections::HashMap; +use std::fs; +use std::path::Path; +use std::path::PathBuf; +use std::process::Command; + +use anyhow::Context; +use anyhow::Result; +use anyhow::anyhow; +use sha1::digest::Output; +use uuid::Uuid; + +use crate::protocol::FileChange; + +const ZERO_OID: &str = "0000000000000000000000000000000000000000"; +const DEV_NULL: &str = "/dev/null"; + +struct BaselineFileInfo { + path: PathBuf, + content: Vec, + mode: FileMode, + oid: String, +} + +/// Tracks sets of changes to files and exposes the overall unified diff. +/// Internally, the way this works is now: +/// 1. Maintain an in-memory baseline snapshot of files when they are first seen. +/// For new additions, do not create a baseline so that diffs are shown as proper additions (using /dev/null). +/// 2. Keep a stable internal filename (uuid) per external path for rename tracking. +/// 3. To compute the aggregated unified diff, compare each baseline snapshot to the current file on disk entirely in-memory +/// using the `similar` crate and emit unified diffs with rewritten external paths. +#[derive(Default)] +pub struct TurnDiffTracker { + /// Map external path -> internal filename (uuid). + external_to_temp_name: HashMap, + /// Internal filename -> baseline file info. + baseline_file_info: HashMap, + /// Internal filename -> external path as of current accumulated state (after applying all changes). + /// This is where renames are tracked. + temp_name_to_current_path: HashMap, + /// Cache of known git worktree roots to avoid repeated filesystem walks. + git_root_cache: Vec, +} + +impl TurnDiffTracker { + pub fn new() -> Self { + Self::default() + } + + /// Front-run apply patch calls to track the starting contents of any modified files. + /// - Creates an in-memory baseline snapshot for files that already exist on disk when first seen. + /// - For additions, we intentionally do not create a baseline snapshot so that diffs are proper additions. + /// - Also updates internal mappings for move/rename events. + pub fn on_patch_begin(&mut self, changes: &HashMap) { + for (path, change) in changes.iter() { + // Ensure a stable internal filename exists for this external path. + if !self.external_to_temp_name.contains_key(path) { + let internal = Uuid::new_v4().to_string(); + self.external_to_temp_name + .insert(path.clone(), internal.clone()); + self.temp_name_to_current_path + .insert(internal.clone(), path.clone()); + + // If the file exists on disk now, snapshot as baseline; else leave missing to represent /dev/null. + let baseline_file_info = if path.exists() { + let mode = file_mode_for_path(path); + let mode_val = mode.unwrap_or(FileMode::Regular); + let content = blob_bytes(path, &mode_val).unwrap_or_default(); + let oid = if mode == Some(FileMode::Symlink) { + format!("{:x}", git_blob_sha1_hex_bytes(&content)) + } else { + self.git_blob_oid_for_path(path) + .unwrap_or_else(|| format!("{:x}", git_blob_sha1_hex_bytes(&content))) + }; + Some(BaselineFileInfo { + path: path.clone(), + content, + mode: mode_val, + oid, + }) + } else { + Some(BaselineFileInfo { + path: path.clone(), + content: vec![], + mode: FileMode::Regular, + oid: ZERO_OID.to_string(), + }) + }; + + if let Some(baseline_file_info) = baseline_file_info { + self.baseline_file_info + .insert(internal.clone(), baseline_file_info); + } + } + + // Track rename/move in current mapping if provided in an Update. + if let FileChange::Update { + move_path: Some(dest), + .. + } = change + { + let uuid_filename = match self.external_to_temp_name.get(path) { + Some(i) => i.clone(), + None => { + // This should be rare, but if we haven't mapped the source, create it with no baseline. + let i = Uuid::new_v4().to_string(); + self.baseline_file_info.insert( + i.clone(), + BaselineFileInfo { + path: path.clone(), + content: vec![], + mode: FileMode::Regular, + oid: ZERO_OID.to_string(), + }, + ); + i + } + }; + // Update current external mapping for temp file name. + self.temp_name_to_current_path + .insert(uuid_filename.clone(), dest.clone()); + // Update forward file_mapping: external current -> internal name. + self.external_to_temp_name.remove(path); + self.external_to_temp_name + .insert(dest.clone(), uuid_filename); + }; + } + } + + fn get_path_for_internal(&self, internal: &str) -> Option { + self.temp_name_to_current_path + .get(internal) + .cloned() + .or_else(|| { + self.baseline_file_info + .get(internal) + .map(|info| info.path.clone()) + }) + } + + /// Find the git worktree root for a file/directory by walking up to the first ancestor containing a `.git` entry. + /// Uses a simple cache of known roots and avoids negative-result caching for simplicity. + fn find_git_root_cached(&mut self, start: &Path) -> Option { + let dir = if start.is_dir() { + start + } else { + start.parent()? + }; + + // Fast path: if any cached root is an ancestor of this path, use it. + if let Some(root) = self + .git_root_cache + .iter() + .find(|r| dir.starts_with(r)) + .cloned() + { + return Some(root); + } + + // Walk up to find a `.git` marker. + let mut cur = dir.to_path_buf(); + loop { + let git_marker = cur.join(".git"); + if git_marker.is_dir() || git_marker.is_file() { + if !self.git_root_cache.iter().any(|r| r == &cur) { + self.git_root_cache.push(cur.clone()); + } + return Some(cur); + } + + // On Windows, avoid walking above the drive or UNC share root. + #[cfg(windows)] + { + if is_windows_drive_or_unc_root(&cur) { + return None; + } + } + + if let Some(parent) = cur.parent() { + cur = parent.to_path_buf(); + } else { + return None; + } + } + } + + /// Return a display string for `path` relative to its git root if found, else absolute. + fn relative_to_git_root_str(&mut self, path: &Path) -> String { + let s = if let Some(root) = self.find_git_root_cached(path) { + if let Ok(rel) = path.strip_prefix(&root) { + rel.display().to_string() + } else { + path.display().to_string() + } + } else { + path.display().to_string() + }; + s.replace('\\', "/") + } + + /// Ask git to compute the blob SHA-1 for the file at `path` within its repository. + /// Returns None if no repository is found or git invocation fails. + fn git_blob_oid_for_path(&mut self, path: &Path) -> Option { + let root = self.find_git_root_cached(path)?; + // Compute a path relative to the repo root for better portability across platforms. + let rel = path.strip_prefix(&root).unwrap_or(path); + let output = Command::new("git") + .arg("-C") + .arg(&root) + .arg("hash-object") + .arg("--") + .arg(rel) + .output() + .ok()?; + if !output.status.success() { + return None; + } + let s = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if s.len() == 40 { Some(s) } else { None } + } + + /// Recompute the aggregated unified diff by comparing all of the in-memory snapshots that were + /// collected before the first time they were touched by apply_patch during this turn with + /// the current repo state. + pub fn get_unified_diff(&mut self) -> Result> { + let mut aggregated = String::new(); + + // Compute diffs per tracked internal file in a stable order by external path. + let mut baseline_file_names: Vec = + self.baseline_file_info.keys().cloned().collect(); + // Sort lexicographically by full repo-relative path to match git behavior. + baseline_file_names.sort_by_key(|internal| { + self.get_path_for_internal(internal) + .map(|p| self.relative_to_git_root_str(&p)) + .unwrap_or_default() + }); + + for internal in baseline_file_names { + aggregated.push_str(self.get_file_diff(&internal).as_str()); + if !aggregated.ends_with('\n') { + aggregated.push('\n'); + } + } + + if aggregated.trim().is_empty() { + Ok(None) + } else { + Ok(Some(aggregated)) + } + } + + fn get_file_diff(&mut self, internal_file_name: &str) -> String { + let mut aggregated = String::new(); + + // Snapshot lightweight fields only. + let (baseline_external_path, baseline_mode, left_oid) = { + if let Some(info) = self.baseline_file_info.get(internal_file_name) { + (info.path.clone(), info.mode, info.oid.clone()) + } else { + (PathBuf::new(), FileMode::Regular, ZERO_OID.to_string()) + } + }; + let current_external_path = match self.get_path_for_internal(internal_file_name) { + Some(p) => p, + None => return aggregated, + }; + + let current_mode = file_mode_for_path(¤t_external_path).unwrap_or(FileMode::Regular); + let right_bytes = blob_bytes(¤t_external_path, ¤t_mode); + + // Compute displays with &mut self before borrowing any baseline content. + let left_display = self.relative_to_git_root_str(&baseline_external_path); + let right_display = self.relative_to_git_root_str(¤t_external_path); + + // Compute right oid before borrowing baseline content. + let right_oid = if let Some(b) = right_bytes.as_ref() { + if current_mode == FileMode::Symlink { + format!("{:x}", git_blob_sha1_hex_bytes(b)) + } else { + self.git_blob_oid_for_path(¤t_external_path) + .unwrap_or_else(|| format!("{:x}", git_blob_sha1_hex_bytes(b))) + } + } else { + ZERO_OID.to_string() + }; + + // Borrow baseline content only after all &mut self uses are done. + let left_present = left_oid.as_str() != ZERO_OID; + let left_bytes: Option<&[u8]> = if left_present { + self.baseline_file_info + .get(internal_file_name) + .map(|i| i.content.as_slice()) + } else { + None + }; + + // Fast path: identical bytes or both missing. + if left_bytes == right_bytes.as_deref() { + return aggregated; + } + + aggregated.push_str(&format!("diff --git a/{left_display} b/{right_display}\n")); + + let is_add = !left_present && right_bytes.is_some(); + let is_delete = left_present && right_bytes.is_none(); + + if is_add { + aggregated.push_str(&format!("new file mode {current_mode}\n")); + } else if is_delete { + aggregated.push_str(&format!("deleted file mode {baseline_mode}\n")); + } else if baseline_mode != current_mode { + aggregated.push_str(&format!("old mode {baseline_mode}\n")); + aggregated.push_str(&format!("new mode {current_mode}\n")); + } + + let left_text = left_bytes.and_then(|b| std::str::from_utf8(b).ok()); + let right_text = right_bytes + .as_deref() + .and_then(|b| std::str::from_utf8(b).ok()); + + let can_text_diff = matches!( + (left_text, right_text, is_add, is_delete), + (Some(_), Some(_), _, _) | (_, Some(_), true, _) | (Some(_), _, _, true) + ); + + if can_text_diff { + let l = left_text.unwrap_or(""); + let r = right_text.unwrap_or(""); + + aggregated.push_str(&format!("index {left_oid}..{right_oid}\n")); + + let old_header = if left_present { + format!("a/{left_display}") + } else { + DEV_NULL.to_string() + }; + let new_header = if right_bytes.is_some() { + format!("b/{right_display}") + } else { + DEV_NULL.to_string() + }; + + let diff = similar::TextDiff::from_lines(l, r); + let unified = diff + .unified_diff() + .context_radius(3) + .header(&old_header, &new_header) + .to_string(); + + aggregated.push_str(&unified); + } else { + aggregated.push_str(&format!("index {left_oid}..{right_oid}\n")); + let old_header = if left_present { + format!("a/{left_display}") + } else { + DEV_NULL.to_string() + }; + let new_header = if right_bytes.is_some() { + format!("b/{right_display}") + } else { + DEV_NULL.to_string() + }; + aggregated.push_str(&format!("--- {old_header}\n")); + aggregated.push_str(&format!("+++ {new_header}\n")); + aggregated.push_str("Binary files differ\n"); + } + aggregated + } +} + +/// Compute the Git SHA-1 blob object ID for the given content (bytes). +fn git_blob_sha1_hex_bytes(data: &[u8]) -> Output { + // Git blob hash is sha1 of: "blob \0" + let header = format!("blob {}\0", data.len()); + use sha1::Digest; + let mut hasher = sha1::Sha1::new(); + hasher.update(header.as_bytes()); + hasher.update(data); + hasher.finalize() +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum FileMode { + Regular, + #[cfg(unix)] + Executable, + Symlink, +} + +impl FileMode { + fn as_str(&self) -> &'static str { + match self { + FileMode::Regular => "100644", + #[cfg(unix)] + FileMode::Executable => "100755", + FileMode::Symlink => "120000", + } + } +} + +impl std::fmt::Display for FileMode { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +#[cfg(unix)] +fn file_mode_for_path(path: &Path) -> Option { + use std::os::unix::fs::PermissionsExt; + let meta = fs::symlink_metadata(path).ok()?; + let ft = meta.file_type(); + if ft.is_symlink() { + return Some(FileMode::Symlink); + } + let mode = meta.permissions().mode(); + let is_exec = (mode & 0o111) != 0; + Some(if is_exec { + FileMode::Executable + } else { + FileMode::Regular + }) +} + +#[cfg(not(unix))] +fn file_mode_for_path(_path: &Path) -> Option { + // Default to non-executable on non-unix. + Some(FileMode::Regular) +} + +fn blob_bytes(path: &Path, mode: &FileMode) -> Option> { + if path.exists() { + let contents = if *mode == FileMode::Symlink { + symlink_blob_bytes(path) + .ok_or_else(|| anyhow!("failed to read symlink target for {}", path.display())) + } else { + fs::read(path) + .with_context(|| format!("failed to read current file for diff {}", path.display())) + }; + contents.ok() + } else { + None + } +} + +#[cfg(unix)] +fn symlink_blob_bytes(path: &Path) -> Option> { + use std::os::unix::ffi::OsStrExt; + let target = std::fs::read_link(path).ok()?; + Some(target.as_os_str().as_bytes().to_vec()) +} + +#[cfg(not(unix))] +fn symlink_blob_bytes(_path: &Path) -> Option> { + None +} + +#[cfg(windows)] +fn is_windows_drive_or_unc_root(p: &std::path::Path) -> bool { + use std::path::Component; + let mut comps = p.components(); + matches!( + (comps.next(), comps.next(), comps.next()), + (Some(Component::Prefix(_)), Some(Component::RootDir), None) + ) +} + +#[cfg(test)] +mod tests { + #![allow(clippy::unwrap_used)] + use super::*; + use pretty_assertions::assert_eq; + use tempfile::tempdir; + + /// Compute the Git SHA-1 blob object ID for the given content (string). + /// This delegates to the bytes version to avoid UTF-8 lossy conversions here. + fn git_blob_sha1_hex(data: &str) -> String { + format!("{:x}", git_blob_sha1_hex_bytes(data.as_bytes())) + } + + fn normalize_diff_for_test(input: &str, root: &Path) -> String { + let root_str = root.display().to_string().replace('\\', "/"); + let replaced = input.replace(&root_str, ""); + // Split into blocks on lines starting with "diff --git ", sort blocks for determinism, and rejoin + let mut blocks: Vec = Vec::new(); + let mut current = String::new(); + for line in replaced.lines() { + if line.starts_with("diff --git ") && !current.is_empty() { + blocks.push(current); + current = String::new(); + } + if !current.is_empty() { + current.push('\n'); + } + current.push_str(line); + } + if !current.is_empty() { + blocks.push(current); + } + blocks.sort(); + let mut out = blocks.join("\n"); + if !out.ends_with('\n') { + out.push('\n'); + } + out + } + + #[test] + fn accumulates_add_and_update() { + let mut acc = TurnDiffTracker::new(); + + let dir = tempdir().unwrap(); + let file = dir.path().join("a.txt"); + + // First patch: add file (baseline should be /dev/null). + let add_changes = HashMap::from([( + file.clone(), + FileChange::Add { + content: "foo\n".to_string(), + }, + )]); + acc.on_patch_begin(&add_changes); + + // Simulate apply: create the file on disk. + fs::write(&file, "foo\n").unwrap(); + let first = acc.get_unified_diff().unwrap().unwrap(); + let first = normalize_diff_for_test(&first, dir.path()); + let expected_first = { + let mode = file_mode_for_path(&file).unwrap_or(FileMode::Regular); + let right_oid = git_blob_sha1_hex("foo\n"); + format!( + r#"diff --git a//a.txt b//a.txt +new file mode {mode} +index {ZERO_OID}..{right_oid} +--- {DEV_NULL} ++++ b//a.txt +@@ -0,0 +1 @@ ++foo +"#, + ) + }; + assert_eq!(first, expected_first); + + // Second patch: update the file on disk. + let update_changes = HashMap::from([( + file.clone(), + FileChange::Update { + unified_diff: "".to_owned(), + move_path: None, + }, + )]); + acc.on_patch_begin(&update_changes); + + // Simulate apply: append a new line. + fs::write(&file, "foo\nbar\n").unwrap(); + let combined = acc.get_unified_diff().unwrap().unwrap(); + let combined = normalize_diff_for_test(&combined, dir.path()); + let expected_combined = { + let mode = file_mode_for_path(&file).unwrap_or(FileMode::Regular); + let right_oid = git_blob_sha1_hex("foo\nbar\n"); + format!( + r#"diff --git a//a.txt b//a.txt +new file mode {mode} +index {ZERO_OID}..{right_oid} +--- {DEV_NULL} ++++ b//a.txt +@@ -0,0 +1,2 @@ ++foo ++bar +"#, + ) + }; + assert_eq!(combined, expected_combined); + } + + #[test] + fn accumulates_delete() { + let dir = tempdir().unwrap(); + let file = dir.path().join("b.txt"); + fs::write(&file, "x\n").unwrap(); + + let mut acc = TurnDiffTracker::new(); + let del_changes = HashMap::from([(file.clone(), FileChange::Delete)]); + acc.on_patch_begin(&del_changes); + + // Simulate apply: delete the file from disk. + let baseline_mode = file_mode_for_path(&file).unwrap_or(FileMode::Regular); + fs::remove_file(&file).unwrap(); + let diff = acc.get_unified_diff().unwrap().unwrap(); + let diff = normalize_diff_for_test(&diff, dir.path()); + let expected = { + let left_oid = git_blob_sha1_hex("x\n"); + format!( + r#"diff --git a//b.txt b//b.txt +deleted file mode {baseline_mode} +index {left_oid}..{ZERO_OID} +--- a//b.txt ++++ {DEV_NULL} +@@ -1 +0,0 @@ +-x +"#, + ) + }; + assert_eq!(diff, expected); + } + + #[test] + fn accumulates_move_and_update() { + let dir = tempdir().unwrap(); + let src = dir.path().join("src.txt"); + let dest = dir.path().join("dst.txt"); + fs::write(&src, "line\n").unwrap(); + + let mut acc = TurnDiffTracker::new(); + let mv_changes = HashMap::from([( + src.clone(), + FileChange::Update { + unified_diff: "".to_owned(), + move_path: Some(dest.clone()), + }, + )]); + acc.on_patch_begin(&mv_changes); + + // Simulate apply: move and update content. + fs::rename(&src, &dest).unwrap(); + fs::write(&dest, "line2\n").unwrap(); + + let out = acc.get_unified_diff().unwrap().unwrap(); + let out = normalize_diff_for_test(&out, dir.path()); + let expected = { + let left_oid = git_blob_sha1_hex("line\n"); + let right_oid = git_blob_sha1_hex("line2\n"); + format!( + r#"diff --git a//src.txt b//dst.txt +index {left_oid}..{right_oid} +--- a//src.txt ++++ b//dst.txt +@@ -1 +1 @@ +-line ++line2 +"# + ) + }; + assert_eq!(out, expected); + } + + #[test] + fn move_without_1change_yields_no_diff() { + let dir = tempdir().unwrap(); + let src = dir.path().join("moved.txt"); + let dest = dir.path().join("renamed.txt"); + fs::write(&src, "same\n").unwrap(); + + let mut acc = TurnDiffTracker::new(); + let mv_changes = HashMap::from([( + src.clone(), + FileChange::Update { + unified_diff: "".to_owned(), + move_path: Some(dest.clone()), + }, + )]); + acc.on_patch_begin(&mv_changes); + + // Simulate apply: move only, no content change. + fs::rename(&src, &dest).unwrap(); + + let diff = acc.get_unified_diff().unwrap(); + assert_eq!(diff, None); + } + + #[test] + fn move_declared_but_file_only_appears_at_dest_is_add() { + let dir = tempdir().unwrap(); + let src = dir.path().join("src.txt"); + let dest = dir.path().join("dest.txt"); + let mut acc = TurnDiffTracker::new(); + let mv = HashMap::from([( + src.clone(), + FileChange::Update { + unified_diff: "".into(), + move_path: Some(dest.clone()), + }, + )]); + acc.on_patch_begin(&mv); + // No file existed initially; create only dest + fs::write(&dest, "hello\n").unwrap(); + let diff = acc.get_unified_diff().unwrap().unwrap(); + let diff = normalize_diff_for_test(&diff, dir.path()); + let expected = { + let mode = file_mode_for_path(&dest).unwrap_or(FileMode::Regular); + let right_oid = git_blob_sha1_hex("hello\n"); + format!( + r#"diff --git a//src.txt b//dest.txt +new file mode {mode} +index {ZERO_OID}..{right_oid} +--- {DEV_NULL} ++++ b//dest.txt +@@ -0,0 +1 @@ ++hello +"#, + ) + }; + assert_eq!(diff, expected); + } + + #[test] + fn update_persists_across_new_baseline_for_new_file() { + let dir = tempdir().unwrap(); + let a = dir.path().join("a.txt"); + let b = dir.path().join("b.txt"); + fs::write(&a, "foo\n").unwrap(); + fs::write(&b, "z\n").unwrap(); + + let mut acc = TurnDiffTracker::new(); + + // First: update existing a.txt (baseline snapshot is created for a). + let update_a = HashMap::from([( + a.clone(), + FileChange::Update { + unified_diff: "".to_owned(), + move_path: None, + }, + )]); + acc.on_patch_begin(&update_a); + // Simulate apply: modify a.txt on disk. + fs::write(&a, "foo\nbar\n").unwrap(); + let first = acc.get_unified_diff().unwrap().unwrap(); + let first = normalize_diff_for_test(&first, dir.path()); + let expected_first = { + let left_oid = git_blob_sha1_hex("foo\n"); + let right_oid = git_blob_sha1_hex("foo\nbar\n"); + format!( + r#"diff --git a//a.txt b//a.txt +index {left_oid}..{right_oid} +--- a//a.txt ++++ b//a.txt +@@ -1 +1,2 @@ + foo ++bar +"# + ) + }; + assert_eq!(first, expected_first); + + // Next: introduce a brand-new path b.txt into baseline snapshots via a delete change. + let del_b = HashMap::from([(b.clone(), FileChange::Delete)]); + acc.on_patch_begin(&del_b); + // Simulate apply: delete b.txt. + let baseline_mode = file_mode_for_path(&b).unwrap_or(FileMode::Regular); + fs::remove_file(&b).unwrap(); + + let combined = acc.get_unified_diff().unwrap().unwrap(); + let combined = normalize_diff_for_test(&combined, dir.path()); + let expected = { + let left_oid_a = git_blob_sha1_hex("foo\n"); + let right_oid_a = git_blob_sha1_hex("foo\nbar\n"); + let left_oid_b = git_blob_sha1_hex("z\n"); + format!( + r#"diff --git a//a.txt b//a.txt +index {left_oid_a}..{right_oid_a} +--- a//a.txt ++++ b//a.txt +@@ -1 +1,2 @@ + foo ++bar +diff --git a//b.txt b//b.txt +deleted file mode {baseline_mode} +index {left_oid_b}..{ZERO_OID} +--- a//b.txt ++++ {DEV_NULL} +@@ -1 +0,0 @@ +-z +"#, + ) + }; + assert_eq!(combined, expected); + } + + #[test] + fn binary_files_differ_update() { + let dir = tempdir().unwrap(); + let file = dir.path().join("bin.dat"); + + // Initial non-UTF8 bytes + let left_bytes: Vec = vec![0xff, 0xfe, 0xfd, 0x00]; + // Updated non-UTF8 bytes + let right_bytes: Vec = vec![0x01, 0x02, 0x03, 0x00]; + + fs::write(&file, &left_bytes).unwrap(); + + let mut acc = TurnDiffTracker::new(); + let update_changes = HashMap::from([( + file.clone(), + FileChange::Update { + unified_diff: "".to_owned(), + move_path: None, + }, + )]); + acc.on_patch_begin(&update_changes); + + // Apply update on disk + fs::write(&file, &right_bytes).unwrap(); + + let diff = acc.get_unified_diff().unwrap().unwrap(); + let diff = normalize_diff_for_test(&diff, dir.path()); + let expected = { + let left_oid = format!("{:x}", git_blob_sha1_hex_bytes(&left_bytes)); + let right_oid = format!("{:x}", git_blob_sha1_hex_bytes(&right_bytes)); + format!( + r#"diff --git a//bin.dat b//bin.dat +index {left_oid}..{right_oid} +--- a//bin.dat ++++ b//bin.dat +Binary files differ +"# + ) + }; + assert_eq!(diff, expected); + } + + #[test] + fn filenames_with_spaces_add_and_update() { + let mut acc = TurnDiffTracker::new(); + + let dir = tempdir().unwrap(); + let file = dir.path().join("name with spaces.txt"); + + // First patch: add file (baseline should be /dev/null). + let add_changes = HashMap::from([( + file.clone(), + FileChange::Add { + content: "foo\n".to_string(), + }, + )]); + acc.on_patch_begin(&add_changes); + + // Simulate apply: create the file on disk. + fs::write(&file, "foo\n").unwrap(); + let first = acc.get_unified_diff().unwrap().unwrap(); + let first = normalize_diff_for_test(&first, dir.path()); + let expected_first = { + let mode = file_mode_for_path(&file).unwrap_or(FileMode::Regular); + let right_oid = git_blob_sha1_hex("foo\n"); + format!( + r#"diff --git a//name with spaces.txt b//name with spaces.txt +new file mode {mode} +index {ZERO_OID}..{right_oid} +--- {DEV_NULL} ++++ b//name with spaces.txt +@@ -0,0 +1 @@ ++foo +"#, + ) + }; + assert_eq!(first, expected_first); + + // Second patch: update the file on disk. + let update_changes = HashMap::from([( + file.clone(), + FileChange::Update { + unified_diff: "".to_owned(), + move_path: None, + }, + )]); + acc.on_patch_begin(&update_changes); + + // Simulate apply: append a new line with a space. + fs::write(&file, "foo\nbar baz\n").unwrap(); + let combined = acc.get_unified_diff().unwrap().unwrap(); + let combined = normalize_diff_for_test(&combined, dir.path()); + let expected_combined = { + let mode = file_mode_for_path(&file).unwrap_or(FileMode::Regular); + let right_oid = git_blob_sha1_hex("foo\nbar baz\n"); + format!( + r#"diff --git a//name with spaces.txt b//name with spaces.txt +new file mode {mode} +index {ZERO_OID}..{right_oid} +--- {DEV_NULL} ++++ b//name with spaces.txt +@@ -0,0 +1,2 @@ ++foo ++bar baz +"#, + ) + }; + assert_eq!(combined, expected_combined); + } +} diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 72e2f9298f..c290d9336b 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -20,6 +20,7 @@ use codex_core::protocol::PatchApplyEndEvent; use codex_core::protocol::SessionConfiguredEvent; use codex_core::protocol::TaskCompleteEvent; use codex_core::protocol::TokenUsage; +use codex_core::protocol::TurnDiffEvent; use owo_colors::OwoColorize; use owo_colors::Style; use shlex::try_join; @@ -399,6 +400,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { stdout, stderr, success, + .. }) => { let patch_begin = self.call_id_to_patch.remove(&call_id); @@ -428,6 +430,10 @@ impl EventProcessor for EventProcessorWithHumanOutput { println!("{}", line.style(self.dimmed)); } } + EventMsg::TurnDiff(TurnDiffEvent { unified_diff }) => { + ts_println!(self, "{}", "turn diff:".style(self.magenta)); + println!("{unified_diff}"); + } EventMsg::ExecApprovalRequest(_) => { // Should we exit? } diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index d489ffe076..205dfa4631 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -263,6 +263,7 @@ async fn run_codex_tool_session_inner( | EventMsg::BackgroundEvent(_) | EventMsg::PatchApplyBegin(_) | EventMsg::PatchApplyEnd(_) + | EventMsg::TurnDiff(_) | EventMsg::GetHistoryEntryResponse(_) | EventMsg::PlanUpdate(_) | EventMsg::ShutdownComplete => { diff --git a/codex-rs/mcp-server/src/conversation_loop.rs b/codex-rs/mcp-server/src/conversation_loop.rs index 534275181a..1db39a2306 100644 --- a/codex-rs/mcp-server/src/conversation_loop.rs +++ b/codex-rs/mcp-server/src/conversation_loop.rs @@ -97,6 +97,7 @@ pub async fn run_conversation_loop( | EventMsg::McpToolCallEnd(_) | EventMsg::ExecCommandBegin(_) | EventMsg::ExecCommandEnd(_) + | EventMsg::TurnDiff(_) | EventMsg::BackgroundEvent(_) | EventMsg::ExecCommandOutputDelta(_) | EventMsg::PatchApplyBegin(_) From dc15a5cf0b50eaa7a75dca754d618090d41d51e3 Mon Sep 17 00:00:00 2001 From: ae Date: Mon, 4 Aug 2025 09:34:46 -0700 Subject: [PATCH 12/18] feat: accept custom instructions in profiles (#1803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allows users to set their experimental_instructions_file in configs. For example the below enables experimental instructions when running `codex -p foo`. ``` [profiles.foo] experimental_instructions_file = "/Users/foo/.codex/prompt.md" ``` # Testing - ✅ Running against a profile with experimental_instructions_file works. - ✅ Running against a profile without experimental_instructions_file works. - ✅ Running against no profile with experimental_instructions_file works. - ✅ Running against no profile without experimental_instructions_file works. --- codex-rs/core/src/config.rs | 10 ++++++---- codex-rs/core/src/config_profile.rs | 2 ++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 87629d821a..b43dc56ba0 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -480,10 +480,12 @@ impl Config { // Load base instructions override from a file if specified. If the // path is relative, resolve it against the effective cwd so the // behaviour matches other path-like config values. - let file_base_instructions = Self::get_base_instructions( - cfg.experimental_instructions_file.as_ref(), - &resolved_cwd, - )?; + let experimental_instructions_path = config_profile + .experimental_instructions_file + .as_ref() + .or(cfg.experimental_instructions_file.as_ref()); + let file_base_instructions = + Self::get_base_instructions(experimental_instructions_path, &resolved_cwd)?; let base_instructions = base_instructions.or(file_base_instructions); let config = Self { diff --git a/codex-rs/core/src/config_profile.rs b/codex-rs/core/src/config_profile.rs index 176a9b1500..d945d1df7a 100644 --- a/codex-rs/core/src/config_profile.rs +++ b/codex-rs/core/src/config_profile.rs @@ -1,4 +1,5 @@ use serde::Deserialize; +use std::path::PathBuf; use crate::config_types::ReasoningEffort; use crate::config_types::ReasoningSummary; @@ -17,4 +18,5 @@ pub struct ConfigProfile { pub model_reasoning_effort: Option, pub model_reasoning_summary: Option, pub chatgpt_base_url: Option, + pub experimental_instructions_file: Option, } From a6139aa0035d19d794a3669d6196f9f32a8c8352 Mon Sep 17 00:00:00 2001 From: easong-openai Date: Mon, 4 Aug 2025 10:42:39 -0700 Subject: [PATCH 13/18] Update prompt.md (#1819) The existing prompt is really bad. As a low-hanging fruit, let's correct the apply_patch instructions - this helps smaller models successfully apply patches. --- codex-rs/core/prompt.md | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/codex-rs/core/prompt.md b/codex-rs/core/prompt.md index 0a4578270a..4e55003b9f 100644 --- a/codex-rs/core/prompt.md +++ b/codex-rs/core/prompt.md @@ -10,7 +10,7 @@ You MUST adhere to the following criteria when executing the task: - Showing user code and tool call details is allowed. - User instructions may overwrite the _CODING GUIDELINES_ section in this developer message. - Do not use \`ls -R\`, \`find\`, or \`grep\` - these are slow in large repos. Use \`rg\` and \`rg --files\`. -- Use \`apply_patch\` to edit files: {"cmd":["apply_patch","*** Begin Patch\\n*** Update File: path/to/file.py\\n@@ def example():\\n- pass\\n+ return 123\\n*** End Patch"]} +- Use \`apply_patch\` to edit files: {"command":["apply_patch","*** Begin Patch\\n*** Update File: path/to/file.py\\n@@ def example():\\n- pass\\n+ return 123\\n*** End Patch"]} - If completing the user's task requires writing or modifying files: - Your code and final answer should follow these _CODING GUIDELINES_: - Fix the problem at the root cause rather than applying surface-level patches, when possible. @@ -40,16 +40,16 @@ You MUST adhere to the following criteria when executing the task: Your patch language is a stripped‑down, file‑oriented diff format designed to be easy to parse and safe to apply. You can think of it as a high‑level envelope: -**_ Begin Patch +*** Begin Patch [ one or more file sections ] -_** End Patch +*** End Patch Within that envelope, you get a sequence of file operations. You MUST include a header to specify the action you are taking. Each operation starts with one of three headers: -**_ Add File: - create a new file. Every following line is a + line (the initial contents). -_** Delete File: - remove an existing file. Nothing follows. +*** Add File: - create a new file. Every following line is a + line (the initial contents). +*** Delete File: - remove an existing file. Nothing follows. \*\*\* Update File: - patch an existing file in place (optionally with a rename). May be immediately followed by \*\*\* Move to: if you want to rename the file. @@ -63,28 +63,28 @@ Within a hunk each line starts with: At the end of a truncated hunk you can emit \*\*\* End of File. Patch := Begin { FileOp } End -Begin := "**_ Begin Patch" NEWLINE -End := "_** End Patch" NEWLINE +Begin := "*** Begin Patch" NEWLINE +End := "*** End Patch" NEWLINE FileOp := AddFile | DeleteFile | UpdateFile -AddFile := "**_ Add File: " path NEWLINE { "+" line NEWLINE } -DeleteFile := "_** Delete File: " path NEWLINE -UpdateFile := "**_ Update File: " path NEWLINE [ MoveTo ] { Hunk } -MoveTo := "_** Move to: " newPath NEWLINE +AddFile := "*** Add File: " path NEWLINE { "+" line NEWLINE } +DeleteFile := "*** Delete File: " path NEWLINE +UpdateFile := "*** Update File: " path NEWLINE [ MoveTo ] { Hunk } +MoveTo := "*** Move to: " newPath NEWLINE Hunk := "@@" [ header ] NEWLINE { HunkLine } [ "*** End of File" NEWLINE ] HunkLine := (" " | "-" | "+") text NEWLINE A full patch can combine several operations: -**_ Begin Patch -_** Add File: hello.txt +*** Begin Patch +*** Add File: hello.txt +Hello world -**_ Update File: src/app.py -_** Move to: src/main.py +*** Update File: src/app.py +*** Move to: src/main.py @@ def greet(): -print("Hi") +print("Hello, world!") -**_ Delete File: obsolete.txt -_** End Patch +*** Delete File: obsolete.txt +*** End Patch It is important to remember: @@ -101,7 +101,7 @@ Plan updates A tool named `update_plan` is available. Use it to keep an up‑to‑date, step‑by‑step plan for the task so you can follow your progress. When making your plans, keep in mind that you are a deployed coding agent - `update_plan` calls should not involve doing anything that you aren't capable of doing. For example, `update_plan` calls should NEVER contain tasks to merge your own pull requests. Only stop to ask the user if you genuinely need their feedback on a change. -- At the start of the task, call `update_plan` with an initial plan: a short list of 1‑sentence steps with a `status` for each step (`pending`, `in_progress`, or `completed`). There should always be exactly one `in_progress` step until everything is done. +- At the start of any nontrivial task, call `update_plan` with an initial plan: a short list of 1‑sentence steps with a `status` for each step (`pending`, `in_progress`, or `completed`). There should always be exactly one `in_progress` step until everything is done. - Whenever you finish a step, call `update_plan` again, marking the finished step as `completed` and the next step as `in_progress`. - If your plan needs to change, call `update_plan` with the revised steps and include an `explanation` describing the change. - When all steps are complete, make a final `update_plan` call with all steps marked `completed`. From 64cfbbd3c8609fb1c1a1bfbc1bf86148e12e6cc4 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Mon, 4 Aug 2025 11:25:01 -0700 Subject: [PATCH 14/18] support more keys in textarea (#1820) Added: * C-m for newline (not sure if this is actually treated differently to Enter, but tui-textarea handles it and it doesn't hurt) * C-d to delete one char forwards (same as Del) * A-bksp to delete backwards one word * A-arrows to navigate by word --- codex-rs/tui/src/bottom_pane/textarea.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs index e150135b75..cb30c2ac7a 100644 --- a/codex-rs/tui/src/bottom_pane/textarea.rs +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -210,7 +210,7 @@ impl TextArea { .. } => self.insert_str(&c.to_string()), KeyEvent { - code: KeyCode::Char('j'), + code: KeyCode::Char('j' | 'm'), modifiers: KeyModifiers::CONTROL, .. } @@ -220,11 +220,22 @@ impl TextArea { } => self.insert_str("\n"), KeyEvent { code: KeyCode::Backspace, + modifiers: KeyModifiers::ALT, + .. + } => self.delete_backward_word(), + KeyEvent { + code: KeyCode::Backspace, + modifiers: KeyModifiers::NONE, .. } => self.delete_backward(1), KeyEvent { code: KeyCode::Delete, .. + } + | KeyEvent { + code: KeyCode::Char('d'), + modifiers: KeyModifiers::CONTROL, + .. } => self.delete_forward(1), KeyEvent { @@ -303,14 +314,14 @@ impl TextArea { } KeyEvent { code: KeyCode::Left, - modifiers: KeyModifiers::CONTROL, + modifiers: KeyModifiers::CONTROL | KeyModifiers::ALT, .. } => { self.set_cursor(self.beginning_of_previous_word()); } KeyEvent { code: KeyCode::Right, - modifiers: KeyModifiers::CONTROL, + modifiers: KeyModifiers::CONTROL | KeyModifiers::ALT, .. } => { self.set_cursor(self.end_of_next_word()); From 2899817c94098caf96009c1d797597df1c298e3f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 4 Aug 2025 14:24:19 -0700 Subject: [PATCH 15/18] chore(deps): bump toml from 0.9.2 to 0.9.4 in /codex-rs (#1815) Bumps [toml](https://github.com/toml-rs/toml) from 0.9.2 to 0.9.4.
Commits

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=toml&package-manager=cargo&previous-version=0.9.2&new-version=0.9.4)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- codex-rs/Cargo.lock | 10 +++++----- codex-rs/core/Cargo.toml | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index eb4eccd897..9d8a027c53 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -661,7 +661,7 @@ dependencies = [ "clap", "codex-core", "serde", - "toml 0.9.2", + "toml 0.9.4", ] [[package]] @@ -707,7 +707,7 @@ dependencies = [ "tokio", "tokio-test", "tokio-util", - "toml 0.9.2", + "toml 0.9.4", "tracing", "tree-sitter", "tree-sitter-bash", @@ -831,7 +831,7 @@ dependencies = [ "tempfile", "tokio", "tokio-test", - "toml 0.9.2", + "toml 0.9.4", "tracing", "tracing-subscriber", "uuid", @@ -4773,9 +4773,9 @@ dependencies = [ [[package]] name = "toml" -version = "0.9.2" +version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ed0aee96c12fa71097902e0bb061a5e1ebd766a6636bb605ba401c45c1650eac" +checksum = "41ae868b5a0f67631c14589f7e250c1ea2c574ee5ba21c6c8dd4b1485705a5a1" dependencies = [ "indexmap 2.10.0", "serde", diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 466e9adf02..e9d6970ded 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -46,7 +46,7 @@ tokio = { version = "1", features = [ "signal", ] } tokio-util = "0.7.14" -toml = "0.9.2" +toml = "0.9.4" tracing = { version = "0.1.41", features = ["log"] } tree-sitter = "0.25.8" tree-sitter-bash = "0.25.0" From 6db597ec0c6833018973bbcbe97139d100913304 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 4 Aug 2025 14:25:00 -0700 Subject: [PATCH 16/18] chore(deps-dev): bump typescript from 5.8.3 to 5.9.2 in /.github/actions/codex (#1814) [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=typescript&package-manager=bun&previous-version=5.8.3&new-version=5.9.2)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/actions/codex/bun.lock | 4 ++-- .github/actions/codex/package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/actions/codex/bun.lock b/.github/actions/codex/bun.lock index 8b546a5ac6..82e12cc4b6 100644 --- a/.github/actions/codex/bun.lock +++ b/.github/actions/codex/bun.lock @@ -11,7 +11,7 @@ "@types/bun": "^1.2.19", "@types/node": "^24.1.0", "prettier": "^3.6.2", - "typescript": "^5.8.3", + "typescript": "^5.9.2", }, }, }, @@ -68,7 +68,7 @@ "tunnel": ["tunnel@0.0.6", "", {}, "sha512-1h/Lnq9yajKY2PEbBadPXj3VxsDDu844OnaAo52UVmIzIvwwtBPIuNvkjuzBlTWpfJyUbG3ez0KSBibQkj4ojg=="], - "typescript": ["typescript@5.8.3", "", { "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" } }, "sha512-p1diW6TqL9L07nNxvRMM7hMMw4c5XOo/1ibL4aAIGmSAt9slTE1Xgw5KWuof2uTOvCg9BY7ZRi+GaF+7sfgPeQ=="], + "typescript": ["typescript@5.9.2", "", { "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" } }, "sha512-CWBzXQrc/qOkhidw1OzBTQuYRbfyxDXJMVJ1XNwUHGROVmuaeiEm3OslpZ1RV96d7SKKjZKrSJu3+t/xlw3R9A=="], "undici": ["undici@5.29.0", "", { "dependencies": { "@fastify/busboy": "^2.0.0" } }, "sha512-raqeBD6NQK4SkWhQzeYKd1KmIG6dllBOTt55Rmkt4HtI9mwdWtJljnrXjAFUBLTSN67HWrOIZ3EPF4kjUw80Bg=="], diff --git a/.github/actions/codex/package.json b/.github/actions/codex/package.json index 21817b8a59..6c7ae9002b 100644 --- a/.github/actions/codex/package.json +++ b/.github/actions/codex/package.json @@ -16,6 +16,6 @@ "@types/bun": "^1.2.19", "@types/node": "^24.1.0", "prettier": "^3.6.2", - "typescript": "^5.8.3" + "typescript": "^5.9.2" } } From 89ab5c3f74f6efcf5ac3b5ddb7390a90aecc9df3 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 4 Aug 2025 14:26:14 -0700 Subject: [PATCH 17/18] chore(deps): bump serde_json from 1.0.141 to 1.0.142 in /codex-rs (#1817) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps [serde_json](https://github.com/serde-rs/json) from 1.0.141 to 1.0.142.
Release notes

Sourced from serde_json's releases.

v1.0.142

Commits

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=serde_json&package-manager=cargo&previous-version=1.0.141&new-version=1.0.142)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- codex-rs/Cargo.lock | 4 ++-- codex-rs/execpolicy/Cargo.toml | 2 +- codex-rs/file-search/Cargo.toml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 9d8a027c53..0ad32c3cd4 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3997,9 +3997,9 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.141" +version = "1.0.142" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "30b9eff21ebe718216c6ec64e1d9ac57087aad11efc64e32002bce4a0d4c03d3" +checksum = "030fedb782600dcbd6f02d479bf0d817ac3bb40d644745b769d6a96bc3afc5a7" dependencies = [ "indexmap 2.10.0", "itoa", diff --git a/codex-rs/execpolicy/Cargo.toml b/codex-rs/execpolicy/Cargo.toml index ad003e66c4..9693d5c41f 100644 --- a/codex-rs/execpolicy/Cargo.toml +++ b/codex-rs/execpolicy/Cargo.toml @@ -26,7 +26,7 @@ multimap = "0.10.0" path-absolutize = "3.1.1" regex-lite = "0.1" serde = { version = "1.0.194", features = ["derive"] } -serde_json = "1.0.110" +serde_json = "1.0.142" serde_with = { version = "3", features = ["macros"] } [dev-dependencies] diff --git a/codex-rs/file-search/Cargo.toml b/codex-rs/file-search/Cargo.toml index 3f70377183..bf1e8e687f 100644 --- a/codex-rs/file-search/Cargo.toml +++ b/codex-rs/file-search/Cargo.toml @@ -17,5 +17,5 @@ clap = { version = "4", features = ["derive"] } ignore = "0.4.23" nucleo-matcher = "0.3.1" serde = { version = "1", features = ["derive"] } -serde_json = "1.0.110" +serde_json = "1.0.142" tokio = { version = "1", features = ["full"] } From 715fb37a047afb7200465fd659a7db8b1cbb1f07 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 4 Aug 2025 15:06:44 -0700 Subject: [PATCH 18/18] [codex] stop printing error message when --output-last-message is not specified --- codex-rs/exec/src/event_processor.rs | 16 ++++++---------- .../src/event_processor_with_human_output.rs | 7 +++---- .../exec/src/event_processor_with_json_output.rs | 7 +++---- 3 files changed, 12 insertions(+), 18 deletions(-) diff --git a/codex-rs/exec/src/event_processor.rs b/codex-rs/exec/src/event_processor.rs index 741f89d7cb..212862a7e5 100644 --- a/codex-rs/exec/src/event_processor.rs +++ b/codex-rs/exec/src/event_processor.rs @@ -44,20 +44,16 @@ pub(crate) fn create_config_summary_entries(config: &Config) -> Vec<(&'static st entries } -pub(crate) fn handle_last_message( - last_agent_message: Option<&str>, - last_message_path: Option<&Path>, -) { - match (last_message_path, last_agent_message) { - (Some(path), Some(msg)) => write_last_message_file(msg, Some(path)), - (Some(path), None) => { - write_last_message_file("", Some(path)); +pub(crate) fn handle_last_message(last_agent_message: Option<&str>, output_file: &Path) { + match last_agent_message { + Some(msg) => write_last_message_file(msg, Some(output_file)), + None => { + write_last_message_file("", Some(output_file)); eprintln!( "Warning: no last agent message; wrote empty content to {}", - path.display() + output_file.display() ); } - (None, _) => eprintln!("Warning: no file to write last message to."), } } diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index c290d9336b..7703c138fc 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -170,10 +170,9 @@ impl EventProcessor for EventProcessorWithHumanOutput { // Ignore. } EventMsg::TaskComplete(TaskCompleteEvent { last_agent_message }) => { - handle_last_message( - last_agent_message.as_deref(), - self.last_message_path.as_deref(), - ); + if let Some(output_file) = self.last_message_path.as_deref() { + handle_last_message(last_agent_message.as_deref(), output_file); + } return CodexStatus::InitiateShutdown; } EventMsg::TokenCount(TokenUsage { total_tokens, .. }) => { diff --git a/codex-rs/exec/src/event_processor_with_json_output.rs b/codex-rs/exec/src/event_processor_with_json_output.rs index e7a658b76f..1d153add6e 100644 --- a/codex-rs/exec/src/event_processor_with_json_output.rs +++ b/codex-rs/exec/src/event_processor_with_json_output.rs @@ -46,10 +46,9 @@ impl EventProcessor for EventProcessorWithJsonOutput { CodexStatus::Running } EventMsg::TaskComplete(TaskCompleteEvent { last_agent_message }) => { - handle_last_message( - last_agent_message.as_deref(), - self.last_message_path.as_deref(), - ); + if let Some(output_file) = self.last_message_path.as_deref() { + handle_last_message(last_agent_message.as_deref(), output_file); + } CodexStatus::InitiateShutdown } EventMsg::ShutdownComplete => CodexStatus::Shutdown,