From d76f96ce797d96a1df19176aa23c0d253ed44ad1 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Jul 2025 09:26:44 -0700 Subject: [PATCH 1/3] fix: support special --codex-run-as-apply-patch arg (#1702) This introduces some special behavior to the CLIs that are using the `codex-arg0` crate where if `arg1` is `--codex-run-as-apply-patch`, then it will run as if `apply_patch arg2` were invoked. This is important because it means we can do things like: ``` SANDBOX_TYPE=landlock # or seatbelt for macOS codex debug "${SANDBOX_TYPE}" -- codex --codex-run-as-apply-patch PATCH ``` which gives us a way to run `apply_patch` while ensuring it adheres to the sandbox the user specified. While it would be nice to use the `arg0` trick like we are currently doing for `codex-linux-sandbox`, there is no way to specify the `arg0` for the underlying command when running under `/usr/bin/sandbox-exec`, so it will not work for us in this case. Admittedly, we could have also supported this via a custom environment variable (e.g., `CODEX_ARG0`), but since environment variables are inherited by child processes, that seemed like a potentially leakier abstraction. This change, as well as our existing reliance on checking `arg0`, place additional requirements on those who include `codex-core`. Its `README.md` has been updated to reflect this. While we could have just added an `apply-patch` subcommand to the `codex` multitool CLI, that would not be sufficient for the standalone `codex-exec` CLI, which is something that we distribute as part of our GitHub releases for those who know they will not be using the TUI and therefore prefer to use a slightly smaller executable: https://github.com/openai/codex/releases/tag/rust-v0.10.0 To that end, this PR adds an integration test to ensure that the `--codex-run-as-apply-patch` option works with the standalone `codex-exec` CLI. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/1702). * #1705 * #1703 * __->__ #1702 * #1698 * #1697 --- codex-rs/Cargo.lock | 4 ++++ codex-rs/arg0/Cargo.toml | 1 + codex-rs/arg0/src/lib.rs | 23 +++++++++++++++++- codex-rs/core/README.md | 17 +++++++++---- codex-rs/exec/Cargo.toml | 5 ++++ codex-rs/exec/tests/apply_patch.rs | 38 ++++++++++++++++++++++++++++++ 6 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 codex-rs/exec/tests/apply_patch.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index f3903d6ea6..653c3e4ef2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -610,6 +610,7 @@ name = "codex-arg0" version = "0.0.0" dependencies = [ "anyhow", + "codex-apply-patch", "codex-core", "codex-linux-sandbox", "dotenvy", @@ -718,14 +719,17 @@ name = "codex-exec" version = "0.0.0" dependencies = [ "anyhow", + "assert_cmd", "chrono", "clap", "codex-arg0", "codex-common", "codex-core", "owo-colors", + "predicates", "serde_json", "shlex", + "tempfile", "tokio", "tracing", "tracing-subscriber", diff --git a/codex-rs/arg0/Cargo.toml b/codex-rs/arg0/Cargo.toml index 9ad1896746..7c55ac0d96 100644 --- a/codex-rs/arg0/Cargo.toml +++ b/codex-rs/arg0/Cargo.toml @@ -12,6 +12,7 @@ workspace = true [dependencies] anyhow = "1" +codex-apply-patch = { path = "../apply-patch" } codex-core = { path = "../core" } codex-linux-sandbox = { path = "../linux-sandbox" } dotenvy = "0.15.7" diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index 624583b8aa..d7109176a5 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -30,7 +30,8 @@ where Fut: Future>, { // Determine if we were invoked via the special alias. - let argv0 = std::env::args_os().next().unwrap_or_default(); + let mut args = std::env::args_os(); + let argv0 = args.next().unwrap_or_default(); let exe_name = Path::new(&argv0) .file_name() .and_then(|s| s.to_str()) @@ -41,6 +42,26 @@ where codex_linux_sandbox::run_main(); } + let argv1 = args.next().unwrap_or_default(); + if argv1 == "--codex-run-as-apply-patch" { + let patch_arg = args.next().and_then(|s| s.to_str().map(|s| s.to_owned())); + let exit_code = match patch_arg { + Some(patch_arg) => { + let mut stdout = std::io::stdout(); + let mut stderr = std::io::stderr(); + match codex_apply_patch::apply_patch(&patch_arg, &mut stdout, &mut stderr) { + Ok(()) => 0, + Err(_) => 1, + } + } + None => { + eprintln!("Error: --codex-run-as-apply-patch requires a UTF-8 PATCH argument."); + 1 + } + }; + std::process::exit(exit_code); + } + // This modifies the environment, which is not thread-safe, so do this // before creating any threads/the Tokio runtime. load_dotenv(); diff --git a/codex-rs/core/README.md b/codex-rs/core/README.md index 9b3e59c8af..9a4c255abe 100644 --- a/codex-rs/core/README.md +++ b/codex-rs/core/README.md @@ -2,9 +2,18 @@ This crate implements the business logic for Codex. It is designed to be used by the various Codex UIs written in Rust. -Though for non-Rust UIs, we are also working to define a _protocol_ for talking to Codex. See: +## Dependencies -- [Specification](../docs/protocol_v1.md) -- [Rust types](./src/protocol.rs) +Note that `codex-core` makes some assumptions about certain helper utilities being available in the environment. Currently, this -You can use the `proto` subcommand using the executable in the [`cli` crate](../cli) to speak the protocol using newline-delimited-JSON over stdin/stdout. +### macOS + +Expects `/usr/bin/sandbox-exec` to be present. + +### Linux + +Expects the binary containing `codex-core` to run the equivalent of `codex debug landlock` when `arg0` is `codex-linux-sandbox`. See the `codex-arg0` crate for details. + +### All Platforms + +Expects the binary containing `codex-core` to simulate the virtual `apply_patch` CLI when `arg1` is `--codex-run-as-apply-patch`. See the `codex-arg0` crate for details. diff --git a/codex-rs/exec/Cargo.toml b/codex-rs/exec/Cargo.toml index c9d94deb5a..ced771f238 100644 --- a/codex-rs/exec/Cargo.toml +++ b/codex-rs/exec/Cargo.toml @@ -37,3 +37,8 @@ tokio = { version = "1", features = [ ] } tracing = { version = "0.1.41", features = ["log"] } tracing-subscriber = { version = "0.3.19", features = ["env-filter"] } + +[dev-dependencies] +assert_cmd = "2" +predicates = "3" +tempfile = "3.13.0" diff --git a/codex-rs/exec/tests/apply_patch.rs b/codex-rs/exec/tests/apply_patch.rs new file mode 100644 index 0000000000..69ac1b8c0a --- /dev/null +++ b/codex-rs/exec/tests/apply_patch.rs @@ -0,0 +1,38 @@ +use anyhow::Context; +use assert_cmd::prelude::*; +use std::fs; +use std::process::Command; +use tempfile::tempdir; + +/// While we may add an `apply-patch` subcommand to the `codex` CLI multitool +/// at some point, we must ensure that the smaller `codex-exec` CLI can still +/// emulate the `apply_patch` CLI. +#[test] +fn test_standalone_exec_cli_can_use_apply_patch() -> anyhow::Result<()> { + let tmp = tempdir()?; + let relative_path = "source.txt"; + let absolute_path = tmp.path().join(relative_path); + fs::write(&absolute_path, "original content\n")?; + + Command::cargo_bin("codex-exec") + .context("should find binary for codex-exec")? + .arg("--codex-run-as-apply-patch") + .arg( + r#"*** Begin Patch +*** Update File: source.txt +@@ +-original content ++modified by apply_patch +*** End Patch"#, + ) + .current_dir(tmp.path()) + .assert() + .success() + .stdout("Success. Updated the following files:\nM source.txt\n") + .stderr(predicates::str::is_empty()); + assert_eq!( + fs::read_to_string(absolute_path)?, + "modified by apply_patch\n" + ); + Ok(()) +} From 527b96a4653224eb9c4011011cda77d6bf5d047f Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Jul 2025 09:26:56 -0700 Subject: [PATCH 2/3] chore: split apply_patch logic out of codex.rs and into apply_patch.rs --- codex-rs/core/src/apply_patch.rs | 406 +++++++++++++++++++++++++++++++ codex-rs/core/src/codex.rs | 400 +----------------------------- codex-rs/core/src/lib.rs | 1 + 3 files changed, 415 insertions(+), 392 deletions(-) create mode 100644 codex-rs/core/src/apply_patch.rs diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs new file mode 100644 index 0000000000..44af72c746 --- /dev/null +++ b/codex-rs/core/src/apply_patch.rs @@ -0,0 +1,406 @@ +use crate::codex::Session; +use crate::models::FunctionCallOutputPayload; +use crate::models::ResponseInputItem; +use crate::protocol::Event; +use crate::protocol::EventMsg; +use crate::protocol::FileChange; +use crate::protocol::PatchApplyBeginEvent; +use crate::protocol::PatchApplyEndEvent; +use crate::protocol::ReviewDecision; +use crate::safety::SafetyCheck; +use crate::safety::assess_patch_safety; +use anyhow::Context; +use codex_apply_patch::AffectedPaths; +use codex_apply_patch::ApplyPatchAction; +use codex_apply_patch::ApplyPatchFileChange; +use codex_apply_patch::print_summary; +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; + +pub(crate) async fn apply_patch( + sess: &Session, + sub_id: String, + call_id: String, + action: ApplyPatchAction, +) -> ResponseInputItem { + let writable_roots_snapshot = { + #[allow(clippy::unwrap_used)] + let guard = sess.writable_roots.lock().unwrap(); + guard.clone() + }; + + let auto_approved = match assess_patch_safety( + &action, + sess.approval_policy, + &writable_roots_snapshot, + &sess.cwd, + ) { + SafetyCheck::AutoApprove { .. } => true, + SafetyCheck::AskUser => { + // Compute a readable summary of path changes to include in the + // approval request so the user can make an informed decision. + let rx_approve = sess + .request_patch_approval(sub_id.clone(), call_id.clone(), &action, None, None) + .await; + match rx_approve.await.unwrap_or_default() { + ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, + ReviewDecision::Denied | ReviewDecision::Abort => { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: "patch rejected by user".to_string(), + success: Some(false), + }, + }; + } + } + } + SafetyCheck::Reject { reason } => { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!("patch rejected: {reason}"), + success: Some(false), + }, + }; + } + }; + + // Verify write permissions before touching the filesystem. + let writable_snapshot = { + #[allow(clippy::unwrap_used)] + sess.writable_roots.lock().unwrap().clone() + }; + + if let Some(offending) = first_offending_path(&action, &writable_snapshot, &sess.cwd) { + let root = offending.parent().unwrap_or(&offending).to_path_buf(); + + let reason = Some(format!( + "grant write access to {} for this session", + root.display() + )); + + let rx = sess + .request_patch_approval( + sub_id.clone(), + call_id.clone(), + &action, + reason.clone(), + Some(root.clone()), + ) + .await; + + if !matches!( + rx.await.unwrap_or_default(), + ReviewDecision::Approved | ReviewDecision::ApprovedForSession + ) { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: "patch rejected by user".to_string(), + success: Some(false), + }, + }; + } + + // user approved, extend writable roots for this session + #[allow(clippy::unwrap_used)] + sess.writable_roots.lock().unwrap().push(root); + } + + let _ = sess + .tx_event + .send(Event { + id: sub_id.clone(), + msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { + call_id: call_id.clone(), + auto_approved, + changes: convert_apply_patch_to_protocol(&action), + }), + }) + .await; + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + // Enforce writable roots. If a write is blocked, collect offending root + // and prompt the user to extend permissions. + let mut result = apply_changes_from_apply_patch_and_report(&action, &mut stdout, &mut stderr); + + if let Err(err) = &result { + if err.kind() == std::io::ErrorKind::PermissionDenied { + // Determine first offending path. + let offending_opt = action + .changes() + .iter() + .flat_map(|(path, change)| match change { + ApplyPatchFileChange::Add { .. } => vec![path.as_ref()], + ApplyPatchFileChange::Delete => vec![path.as_ref()], + ApplyPatchFileChange::Update { + move_path: Some(move_path), + .. + } => { + vec![path.as_ref(), move_path.as_ref()] + } + ApplyPatchFileChange::Update { + move_path: None, .. + } => vec![path.as_ref()], + }) + .find_map(|path: &Path| { + // ApplyPatchAction promises to guarantee absolute paths. + if !path.is_absolute() { + panic!("apply_patch invariant failed: path is not absolute: {path:?}"); + } + + let writable = { + #[allow(clippy::unwrap_used)] + let roots = sess.writable_roots.lock().unwrap(); + roots.iter().any(|root| path.starts_with(root)) + }; + if writable { + None + } else { + Some(path.to_path_buf()) + } + }); + + if let Some(offending) = offending_opt { + let root = offending.parent().unwrap_or(&offending).to_path_buf(); + + let reason = Some(format!( + "grant write access to {} for this session", + root.display() + )); + let rx = sess + .request_patch_approval( + sub_id.clone(), + call_id.clone(), + &action, + reason.clone(), + Some(root.clone()), + ) + .await; + if matches!( + rx.await.unwrap_or_default(), + ReviewDecision::Approved | ReviewDecision::ApprovedForSession + ) { + // Extend writable roots. + #[allow(clippy::unwrap_used)] + sess.writable_roots.lock().unwrap().push(root); + stdout.clear(); + stderr.clear(); + result = apply_changes_from_apply_patch_and_report( + &action, + &mut stdout, + &mut stderr, + ); + } + } + } + } + + // Emit PatchApplyEnd event. + let success_flag = result.is_ok(); + let _ = sess + .tx_event + .send(Event { + id: sub_id.clone(), + msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { + call_id: call_id.clone(), + stdout: String::from_utf8_lossy(&stdout).to_string(), + stderr: String::from_utf8_lossy(&stderr).to_string(), + success: success_flag, + }), + }) + .await; + + match result { + Ok(_) => ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: String::from_utf8_lossy(&stdout).to_string(), + success: None, + }, + }, + Err(e) => ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!("error: {e:#}, stderr: {}", String::from_utf8_lossy(&stderr)), + success: Some(false), + }, + }, + } +} + +/// Return the first path in `hunks` that is NOT under any of the +/// `writable_roots` (after normalising). If all paths are acceptable, +/// returns None. +fn first_offending_path( + action: &ApplyPatchAction, + writable_roots: &[PathBuf], + cwd: &Path, +) -> Option { + let changes = action.changes(); + for (path, change) in changes { + let candidate = match change { + ApplyPatchFileChange::Add { .. } => path, + ApplyPatchFileChange::Delete => path, + ApplyPatchFileChange::Update { move_path, .. } => move_path.as_ref().unwrap_or(path), + }; + + let abs = if candidate.is_absolute() { + candidate.clone() + } else { + cwd.join(candidate) + }; + + let mut allowed = false; + for root in writable_roots { + let root_abs = if root.is_absolute() { + root.clone() + } else { + cwd.join(root) + }; + if abs.starts_with(&root_abs) { + allowed = true; + break; + } + } + + if !allowed { + return Some(candidate.clone()); + } + } + None +} + +pub(crate) fn convert_apply_patch_to_protocol( + action: &ApplyPatchAction, +) -> HashMap { + let changes = action.changes(); + let mut result = HashMap::with_capacity(changes.len()); + for (path, change) in changes { + let protocol_change = match change { + ApplyPatchFileChange::Add { content } => FileChange::Add { + content: content.clone(), + }, + ApplyPatchFileChange::Delete => FileChange::Delete, + ApplyPatchFileChange::Update { + unified_diff, + move_path, + new_content: _new_content, + } => FileChange::Update { + unified_diff: unified_diff.clone(), + move_path: move_path.clone(), + }, + }; + result.insert(path.clone(), protocol_change); + } + result +} + +fn apply_changes_from_apply_patch_and_report( + action: &ApplyPatchAction, + stdout: &mut impl std::io::Write, + stderr: &mut impl std::io::Write, +) -> std::io::Result<()> { + match apply_changes_from_apply_patch(action) { + Ok(affected_paths) => { + print_summary(&affected_paths, stdout)?; + } + Err(err) => { + writeln!(stderr, "{err:?}")?; + } + } + + Ok(()) +} + +fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result { + let mut added: Vec = Vec::new(); + let mut modified: Vec = Vec::new(); + let mut deleted: Vec = Vec::new(); + + let changes = action.changes(); + for (path, change) in changes { + match change { + ApplyPatchFileChange::Add { content } => { + if let Some(parent) = path.parent() { + if !parent.as_os_str().is_empty() { + std::fs::create_dir_all(parent).with_context(|| { + format!("Failed to create parent directories for {}", path.display()) + })?; + } + } + std::fs::write(path, content) + .with_context(|| format!("Failed to write file {}", path.display()))?; + added.push(path.clone()); + } + ApplyPatchFileChange::Delete => { + std::fs::remove_file(path) + .with_context(|| format!("Failed to delete file {}", path.display()))?; + deleted.push(path.clone()); + } + ApplyPatchFileChange::Update { + unified_diff: _unified_diff, + move_path, + new_content, + } => { + if let Some(move_path) = move_path { + if let Some(parent) = move_path.parent() { + if !parent.as_os_str().is_empty() { + std::fs::create_dir_all(parent).with_context(|| { + format!( + "Failed to create parent directories for {}", + move_path.display() + ) + })?; + } + } + + std::fs::rename(path, move_path) + .with_context(|| format!("Failed to rename file {}", path.display()))?; + std::fs::write(move_path, new_content)?; + modified.push(move_path.clone()); + deleted.push(path.clone()); + } else { + std::fs::write(path, new_content)?; + modified.push(path.clone()); + } + } + } + } + + Ok(AffectedPaths { + added, + modified, + deleted, + }) +} + +pub(crate) fn get_writable_roots(cwd: &Path) -> Vec { + let mut writable_roots = Vec::new(); + if cfg!(target_os = "macos") { + // On macOS, $TMPDIR is private to the user. + writable_roots.push(std::env::temp_dir()); + + // Allow pyenv to update its shims directory. Without this, any tool + // that happens to be managed by `pyenv` will fail with an error like: + // + // pyenv: cannot rehash: $HOME/.pyenv/shims isn't writable + // + // which is emitted every time `pyenv` tries to run `rehash` (for + // example, after installing a new Python package that drops an entry + // point). Although the sandbox is intentionally read‑only by default, + // writing to the user's local `pyenv` directory is safe because it + // is already user‑writable and scoped to the current user account. + if let Ok(home_dir) = std::env::var("HOME") { + let pyenv_dir = PathBuf::from(home_dir).join(".pyenv"); + writable_roots.push(pyenv_dir); + } + } + + writable_roots.push(cwd.to_path_buf()); + + writable_roots +} diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 5764440e79..3ab3e8d780 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4,22 +4,17 @@ use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; -use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; use std::sync::atomic::AtomicU64; use std::time::Duration; -use anyhow::Context; use async_channel::Receiver; use async_channel::Sender; -use codex_apply_patch::AffectedPaths; use codex_apply_patch::ApplyPatchAction; -use codex_apply_patch::ApplyPatchFileChange; use codex_apply_patch::MaybeApplyPatchVerified; use codex_apply_patch::maybe_parse_apply_patch_verified; -use codex_apply_patch::print_summary; use futures::prelude::*; use mcp_types::CallToolResult; use serde::Serialize; @@ -34,6 +29,9 @@ use tracing::trace; use tracing::warn; use uuid::Uuid; +use crate::apply_patch::convert_apply_patch_to_protocol; +use crate::apply_patch::get_writable_roots; +use crate::apply_patch::{self}; use crate::client::ModelClient; use crate::client_common::Prompt; use crate::client_common::ResponseEvent; @@ -71,11 +69,8 @@ use crate::protocol::EventMsg; use crate::protocol::ExecApprovalRequestEvent; use crate::protocol::ExecCommandBeginEvent; use crate::protocol::ExecCommandEndEvent; -use crate::protocol::FileChange; use crate::protocol::InputItem; use crate::protocol::Op; -use crate::protocol::PatchApplyBeginEvent; -use crate::protocol::PatchApplyEndEvent; use crate::protocol::ReviewDecision; use crate::protocol::SandboxPolicy; use crate::protocol::SessionConfiguredEvent; @@ -84,7 +79,6 @@ use crate::protocol::TaskCompleteEvent; use crate::rollout::RolloutRecorder; use crate::safety::SafetyCheck; use crate::safety::assess_command_safety; -use crate::safety::assess_patch_safety; use crate::shell; use crate::user_notification::UserNotification; use crate::util::backoff; @@ -189,19 +183,19 @@ impl Codex { /// A session has at most 1 running task at a time, and can be interrupted by user input. pub(crate) struct Session { client: ModelClient, - tx_event: Sender, + pub(crate) tx_event: Sender, ctrl_c: Arc, /// The session's current working directory. All relative paths provided by /// the model as well as sandbox policies are resolved against this path /// instead of `std::env::current_dir()`. - cwd: PathBuf, + pub(crate) cwd: PathBuf, base_instructions: Option, user_instructions: Option, - approval_policy: AskForApproval, + pub(crate) approval_policy: AskForApproval, sandbox_policy: SandboxPolicy, shell_environment_policy: ShellEnvironmentPolicy, - writable_roots: Mutex>, + pub(crate) writable_roots: Mutex>, disable_response_storage: bool, /// Manager for external MCP servers/tools. @@ -1419,7 +1413,7 @@ async fn handle_container_exec_with_params( // check if this was a patch, and apply it if so match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { MaybeApplyPatchVerified::Body(changes) => { - return apply_patch(sess, sub_id, call_id, changes).await; + return apply_patch::apply_patch(sess, sub_id, call_id, changes).await; } MaybeApplyPatchVerified::CorrectnessError(parse_error) => { // It looks like an invocation of `apply_patch`, but we @@ -1668,384 +1662,6 @@ async fn handle_sandbox_error( } } -async fn apply_patch( - sess: &Session, - sub_id: String, - call_id: String, - action: ApplyPatchAction, -) -> ResponseInputItem { - let writable_roots_snapshot = { - let guard = sess.writable_roots.lock().unwrap(); - guard.clone() - }; - - let auto_approved = match assess_patch_safety( - &action, - sess.approval_policy, - &writable_roots_snapshot, - &sess.cwd, - ) { - SafetyCheck::AutoApprove { .. } => true, - SafetyCheck::AskUser => { - // Compute a readable summary of path changes to include in the - // approval request so the user can make an informed decision. - let rx_approve = sess - .request_patch_approval(sub_id.clone(), call_id.clone(), &action, None, None) - .await; - match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, - ReviewDecision::Denied | ReviewDecision::Abort => { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: "patch rejected by user".to_string(), - success: Some(false), - }, - }; - } - } - } - SafetyCheck::Reject { reason } => { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("patch rejected: {reason}"), - success: Some(false), - }, - }; - } - }; - - // Verify write permissions before touching the filesystem. - let writable_snapshot = { sess.writable_roots.lock().unwrap().clone() }; - - if let Some(offending) = first_offending_path(&action, &writable_snapshot, &sess.cwd) { - let root = offending.parent().unwrap_or(&offending).to_path_buf(); - - let reason = Some(format!( - "grant write access to {} for this session", - root.display() - )); - - let rx = sess - .request_patch_approval( - sub_id.clone(), - call_id.clone(), - &action, - reason.clone(), - Some(root.clone()), - ) - .await; - - if !matches!( - rx.await.unwrap_or_default(), - ReviewDecision::Approved | ReviewDecision::ApprovedForSession - ) { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: "patch rejected by user".to_string(), - success: Some(false), - }, - }; - } - - // user approved, extend writable roots for this session - sess.writable_roots.lock().unwrap().push(root); - } - - let _ = sess - .tx_event - .send(Event { - id: sub_id.clone(), - msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { - call_id: call_id.clone(), - auto_approved, - changes: convert_apply_patch_to_protocol(&action), - }), - }) - .await; - - let mut stdout = Vec::new(); - let mut stderr = Vec::new(); - // Enforce writable roots. If a write is blocked, collect offending root - // and prompt the user to extend permissions. - let mut result = apply_changes_from_apply_patch_and_report(&action, &mut stdout, &mut stderr); - - if let Err(err) = &result { - if err.kind() == std::io::ErrorKind::PermissionDenied { - // Determine first offending path. - let offending_opt = action - .changes() - .iter() - .flat_map(|(path, change)| match change { - ApplyPatchFileChange::Add { .. } => vec![path.as_ref()], - ApplyPatchFileChange::Delete => vec![path.as_ref()], - ApplyPatchFileChange::Update { - move_path: Some(move_path), - .. - } => { - vec![path.as_ref(), move_path.as_ref()] - } - ApplyPatchFileChange::Update { - move_path: None, .. - } => vec![path.as_ref()], - }) - .find_map(|path: &Path| { - // ApplyPatchAction promises to guarantee absolute paths. - if !path.is_absolute() { - panic!("apply_patch invariant failed: path is not absolute: {path:?}"); - } - - let writable = { - let roots = sess.writable_roots.lock().unwrap(); - roots.iter().any(|root| path.starts_with(root)) - }; - if writable { - None - } else { - Some(path.to_path_buf()) - } - }); - - if let Some(offending) = offending_opt { - let root = offending.parent().unwrap_or(&offending).to_path_buf(); - - let reason = Some(format!( - "grant write access to {} for this session", - root.display() - )); - let rx = sess - .request_patch_approval( - sub_id.clone(), - call_id.clone(), - &action, - reason.clone(), - Some(root.clone()), - ) - .await; - if matches!( - rx.await.unwrap_or_default(), - ReviewDecision::Approved | ReviewDecision::ApprovedForSession - ) { - // Extend writable roots. - sess.writable_roots.lock().unwrap().push(root); - stdout.clear(); - stderr.clear(); - result = apply_changes_from_apply_patch_and_report( - &action, - &mut stdout, - &mut stderr, - ); - } - } - } - } - - // Emit PatchApplyEnd event. - let success_flag = result.is_ok(); - let _ = sess - .tx_event - .send(Event { - id: sub_id.clone(), - msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { - call_id: call_id.clone(), - stdout: String::from_utf8_lossy(&stdout).to_string(), - stderr: String::from_utf8_lossy(&stderr).to_string(), - success: success_flag, - }), - }) - .await; - - match result { - Ok(_) => ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: String::from_utf8_lossy(&stdout).to_string(), - success: None, - }, - }, - Err(e) => ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("error: {e:#}, stderr: {}", String::from_utf8_lossy(&stderr)), - success: Some(false), - }, - }, - } -} - -/// Return the first path in `hunks` that is NOT under any of the -/// `writable_roots` (after normalising). If all paths are acceptable, -/// returns None. -fn first_offending_path( - action: &ApplyPatchAction, - writable_roots: &[PathBuf], - cwd: &Path, -) -> Option { - let changes = action.changes(); - for (path, change) in changes { - let candidate = match change { - ApplyPatchFileChange::Add { .. } => path, - ApplyPatchFileChange::Delete => path, - ApplyPatchFileChange::Update { move_path, .. } => move_path.as_ref().unwrap_or(path), - }; - - let abs = if candidate.is_absolute() { - candidate.clone() - } else { - cwd.join(candidate) - }; - - let mut allowed = false; - for root in writable_roots { - let root_abs = if root.is_absolute() { - root.clone() - } else { - cwd.join(root) - }; - if abs.starts_with(&root_abs) { - allowed = true; - break; - } - } - - if !allowed { - return Some(candidate.clone()); - } - } - None -} - -fn convert_apply_patch_to_protocol(action: &ApplyPatchAction) -> HashMap { - let changes = action.changes(); - let mut result = HashMap::with_capacity(changes.len()); - for (path, change) in changes { - let protocol_change = match change { - ApplyPatchFileChange::Add { content } => FileChange::Add { - content: content.clone(), - }, - ApplyPatchFileChange::Delete => FileChange::Delete, - ApplyPatchFileChange::Update { - unified_diff, - move_path, - new_content: _new_content, - } => FileChange::Update { - unified_diff: unified_diff.clone(), - move_path: move_path.clone(), - }, - }; - result.insert(path.clone(), protocol_change); - } - result -} - -fn apply_changes_from_apply_patch_and_report( - action: &ApplyPatchAction, - stdout: &mut impl std::io::Write, - stderr: &mut impl std::io::Write, -) -> std::io::Result<()> { - match apply_changes_from_apply_patch(action) { - Ok(affected_paths) => { - print_summary(&affected_paths, stdout)?; - } - Err(err) => { - writeln!(stderr, "{err:?}")?; - } - } - - Ok(()) -} - -fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result { - let mut added: Vec = Vec::new(); - let mut modified: Vec = Vec::new(); - let mut deleted: Vec = Vec::new(); - - let changes = action.changes(); - for (path, change) in changes { - match change { - ApplyPatchFileChange::Add { content } => { - if let Some(parent) = path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent).with_context(|| { - format!("Failed to create parent directories for {}", path.display()) - })?; - } - } - std::fs::write(path, content) - .with_context(|| format!("Failed to write file {}", path.display()))?; - added.push(path.clone()); - } - ApplyPatchFileChange::Delete => { - std::fs::remove_file(path) - .with_context(|| format!("Failed to delete file {}", path.display()))?; - deleted.push(path.clone()); - } - ApplyPatchFileChange::Update { - unified_diff: _unified_diff, - move_path, - new_content, - } => { - if let Some(move_path) = move_path { - if let Some(parent) = move_path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent).with_context(|| { - format!( - "Failed to create parent directories for {}", - move_path.display() - ) - })?; - } - } - - std::fs::rename(path, move_path) - .with_context(|| format!("Failed to rename file {}", path.display()))?; - std::fs::write(move_path, new_content)?; - modified.push(move_path.clone()); - deleted.push(path.clone()); - } else { - std::fs::write(path, new_content)?; - modified.push(path.clone()); - } - } - } - } - - Ok(AffectedPaths { - added, - modified, - deleted, - }) -} - -fn get_writable_roots(cwd: &Path) -> Vec { - let mut writable_roots = Vec::new(); - if cfg!(target_os = "macos") { - // On macOS, $TMPDIR is private to the user. - writable_roots.push(std::env::temp_dir()); - - // Allow pyenv to update its shims directory. Without this, any tool - // that happens to be managed by `pyenv` will fail with an error like: - // - // pyenv: cannot rehash: $HOME/.pyenv/shims isn't writable - // - // which is emitted every time `pyenv` tries to run `rehash` (for - // example, after installing a new Python package that drops an entry - // point). Although the sandbox is intentionally read‑only by default, - // writing to the user's local `pyenv` directory is safe because it - // is already user‑writable and scoped to the current user account. - if let Ok(home_dir) = std::env::var("HOME") { - let pyenv_dir = PathBuf::from(home_dir).join(".pyenv"); - writable_roots.push(pyenv_dir); - } - } - - writable_roots.push(cwd.to_path_buf()); - - writable_roots -} - /// Exec output is a pre-serialized JSON payload fn format_exec_output(output: &str, exit_code: i32, duration: Duration) -> String { #[derive(Serialize)] diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index f390038c0c..6cb6aaa629 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -5,6 +5,7 @@ // the TUI or the tracing stack). #![deny(clippy::print_stdout, clippy::print_stderr)] +mod apply_patch; mod bash; mod chat_completions; mod client; From 787b7a7ca436245fabd230563a8819b5b8bed44e Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Jul 2025 09:26:56 -0700 Subject: [PATCH 3/3] fix: run apply_patch calls through the sandbox --- codex-rs/apply-patch/src/lib.rs | 62 ++++++++--- codex-rs/apply-patch/src/parser.rs | 54 +++++++--- codex-rs/core/src/apply_patch.rs | 63 +++++++---- codex-rs/core/src/codex.rs | 165 ++++++++++++++++++----------- 4 files changed, 226 insertions(+), 118 deletions(-) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index c81241d0da..1207f8830f 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -58,16 +58,22 @@ impl PartialEq for IoError { #[derive(Debug, PartialEq)] pub enum MaybeApplyPatch { - Body(Vec), + Body(ApplyPatchSource), ShellParseError(ExtractHeredocError), PatchParseError(ParseError), NotApplyPatch, } +#[derive(Debug, PartialEq)] +pub struct ApplyPatchSource { + pub hunks: Vec, + pub patch: String, +} + pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { match argv { [cmd, body] if cmd == "apply_patch" => match parse_patch(body) { - Ok(hunks) => MaybeApplyPatch::Body(hunks), + Ok(source) => MaybeApplyPatch::Body(source), Err(e) => MaybeApplyPatch::PatchParseError(e), }, [bash, flag, script] @@ -77,7 +83,7 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { { match extract_heredoc_body_from_apply_patch_command(script) { Ok(body) => match parse_patch(&body) { - Ok(hunks) => MaybeApplyPatch::Body(hunks), + Ok(source) => MaybeApplyPatch::Body(source), Err(e) => MaybeApplyPatch::PatchParseError(e), }, Err(e) => MaybeApplyPatch::ShellParseError(e), @@ -121,6 +127,14 @@ pub enum MaybeApplyPatchVerified { /// construction, all paths should be absolute paths. pub struct ApplyPatchAction { changes: HashMap, + + /// The raw patch argument that can be used with `apply_patch` as an exec + /// call. i.e., if the original arg was parsed in "lenient" mode with a + /// heredoc, this should be the value without the heredoc wrapper. + pub patch: String, + + /// The working directory that was used to resolve relative paths in the patch. + pub cwd: PathBuf, } impl ApplyPatchAction { @@ -140,8 +154,22 @@ impl ApplyPatchAction { panic!("path must be absolute"); } + #[allow(clippy::unwrap_used)] + let filename = path.file_name().unwrap().to_string_lossy(); + let patch = format!( + r#"*** Begin Patch +*** Update File: {filename} +@@ ++ {content} +*** End Patch"#, + ); let changes = HashMap::from([(path.to_path_buf(), ApplyPatchFileChange::Add { content })]); - Self { changes } + #[allow(clippy::unwrap_used)] + Self { + changes, + cwd: path.parent().unwrap().to_path_buf(), + patch, + } } } @@ -149,7 +177,7 @@ impl ApplyPatchAction { /// patch. pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApplyPatchVerified { match maybe_parse_apply_patch(argv) { - MaybeApplyPatch::Body(hunks) => { + MaybeApplyPatch::Body(ApplyPatchSource { patch, hunks }) => { let mut changes = HashMap::new(); for hunk in hunks { let path = hunk.resolve_path(cwd); @@ -183,7 +211,11 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp } } } - MaybeApplyPatchVerified::Body(ApplyPatchAction { changes }) + MaybeApplyPatchVerified::Body(ApplyPatchAction { + changes, + patch, + cwd: cwd.to_path_buf(), + }) } MaybeApplyPatch::ShellParseError(e) => MaybeApplyPatchVerified::ShellParseError(e), MaybeApplyPatch::PatchParseError(e) => MaybeApplyPatchVerified::CorrectnessError(e.into()), @@ -264,7 +296,7 @@ pub fn apply_patch( stderr: &mut impl std::io::Write, ) -> Result<(), ApplyPatchError> { let hunks = match parse_patch(patch) { - Ok(hunks) => hunks, + Ok(source) => source.hunks, Err(e) => { match &e { InvalidPatchError(message) => { @@ -652,7 +684,7 @@ mod tests { ]); match maybe_parse_apply_patch(&args) { - MaybeApplyPatch::Body(hunks) => { + MaybeApplyPatch::Body(ApplyPatchSource { hunks, patch: _ }) => { assert_eq!( hunks, vec![Hunk::AddFile { @@ -679,7 +711,7 @@ PATCH"#, ]); match maybe_parse_apply_patch(&args) { - MaybeApplyPatch::Body(hunks) => { + MaybeApplyPatch::Body(ApplyPatchSource { hunks, patch: _ }) => { assert_eq!( hunks, vec![Hunk::AddFile { @@ -954,7 +986,7 @@ PATCH"#, )); let patch = parse_patch(&patch).unwrap(); - let update_file_chunks = match patch.as_slice() { + let update_file_chunks = match patch.hunks.as_slice() { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; @@ -992,7 +1024,7 @@ PATCH"#, )); let patch = parse_patch(&patch).unwrap(); - let chunks = match patch.as_slice() { + let chunks = match patch.hunks.as_slice() { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; @@ -1029,7 +1061,7 @@ PATCH"#, )); let patch = parse_patch(&patch).unwrap(); - let chunks = match patch.as_slice() { + let chunks = match patch.hunks.as_slice() { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; @@ -1064,7 +1096,7 @@ PATCH"#, )); let patch = parse_patch(&patch).unwrap(); - let chunks = match patch.as_slice() { + let chunks = match patch.hunks.as_slice() { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; @@ -1110,7 +1142,7 @@ PATCH"#, // Extract chunks then build the unified diff. let parsed = parse_patch(&patch).unwrap(); - let chunks = match parsed.as_slice() { + let chunks = match parsed.hunks.as_slice() { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; @@ -1193,6 +1225,8 @@ g new_content: "updated session directory content\n".to_string(), }, )]), + patch: argv[1].clone(), + cwd: session_dir.path().to_path_buf(), }) ); } diff --git a/codex-rs/apply-patch/src/parser.rs b/codex-rs/apply-patch/src/parser.rs index d07691a49d..edd622728e 100644 --- a/codex-rs/apply-patch/src/parser.rs +++ b/codex-rs/apply-patch/src/parser.rs @@ -86,6 +86,8 @@ impl Hunk { use Hunk::*; +use crate::ApplyPatchSource; + #[derive(Debug, PartialEq, Clone)] pub struct UpdateFileChunk { /// A single line of context used to narrow down the position of the chunk @@ -102,7 +104,7 @@ pub struct UpdateFileChunk { pub is_end_of_file: bool, } -pub fn parse_patch(patch: &str) -> Result, ParseError> { +pub fn parse_patch(patch: &str) -> Result { let mode = if PARSE_IN_STRICT_MODE { ParseMode::Strict } else { @@ -150,7 +152,7 @@ enum ParseMode { Lenient, } -fn parse_patch_text(patch: &str, mode: ParseMode) -> Result, ParseError> { +fn parse_patch_text(patch: &str, mode: ParseMode) -> Result { let lines: Vec<&str> = patch.trim().lines().collect(); let lines: &[&str] = match check_patch_boundaries_strict(&lines) { Ok(()) => &lines, @@ -173,7 +175,8 @@ fn parse_patch_text(patch: &str, mode: ParseMode) -> Result, ParseErro line_number += hunk_lines; remaining_lines = &remaining_lines[hunk_lines..] } - Ok(hunks) + let patch = lines.join("\n"); + Ok(ApplyPatchSource { hunks, patch }) } /// Checks the start and end lines of the patch text for `apply_patch`, @@ -455,8 +458,10 @@ fn test_parse_patch() { "*** Begin Patch\n\ *** End Patch", ParseMode::Strict - ), - Ok(Vec::new()) + ) + .unwrap() + .hunks, + Vec::new() ); assert_eq!( parse_patch_text( @@ -472,8 +477,10 @@ fn test_parse_patch() { + return 123\n\ *** End Patch", ParseMode::Strict - ), - Ok(vec![ + ) + .unwrap() + .hunks, + vec![ AddFile { path: PathBuf::from("path/add.py"), contents: "abc\ndef\n".to_string() @@ -491,7 +498,7 @@ fn test_parse_patch() { is_end_of_file: false }] } - ]) + ] ); // Update hunk followed by another hunk (Add File). assert_eq!( @@ -504,8 +511,10 @@ fn test_parse_patch() { +content\n\ *** End Patch", ParseMode::Strict - ), - Ok(vec![ + ) + .unwrap() + .hunks, + vec![ UpdateFile { path: PathBuf::from("file.py"), move_path: None, @@ -520,7 +529,7 @@ fn test_parse_patch() { path: PathBuf::from("other.py"), contents: "content\n".to_string() } - ]) + ] ); // Update hunk without an explicit @@ header for the first chunk should parse. @@ -533,8 +542,10 @@ fn test_parse_patch() { +bar *** End Patch"#, ParseMode::Strict - ), - Ok(vec![UpdateFile { + ) + .unwrap() + .hunks, + vec![UpdateFile { path: PathBuf::from("file2.py"), move_path: None, chunks: vec![UpdateFileChunk { @@ -543,7 +554,7 @@ fn test_parse_patch() { new_lines: vec!["import foo".to_string(), "bar".to_string()], is_end_of_file: false, }], - }]) + }] ); } @@ -574,7 +585,10 @@ fn test_parse_patch_lenient() { ); assert_eq!( parse_patch_text(&patch_text_in_heredoc, ParseMode::Lenient), - Ok(expected_patch.clone()) + Ok(ApplyPatchSource { + hunks: expected_patch.clone(), + patch: patch_text.to_string() + }) ); let patch_text_in_single_quoted_heredoc = format!("<<'EOF'\n{patch_text}\nEOF\n"); @@ -584,7 +598,10 @@ fn test_parse_patch_lenient() { ); assert_eq!( parse_patch_text(&patch_text_in_single_quoted_heredoc, ParseMode::Lenient), - Ok(expected_patch.clone()) + Ok(ApplyPatchSource { + hunks: expected_patch.clone(), + patch: patch_text.to_string() + }) ); let patch_text_in_double_quoted_heredoc = format!("<<\"EOF\"\n{patch_text}\nEOF\n"); @@ -594,7 +611,10 @@ fn test_parse_patch_lenient() { ); assert_eq!( parse_patch_text(&patch_text_in_double_quoted_heredoc, ParseMode::Lenient), - Ok(expected_patch.clone()) + Ok(ApplyPatchSource { + hunks: expected_patch.clone(), + patch: patch_text.to_string() + }) ); let patch_text_in_mismatched_quotes_heredoc = format!("<<\"EOF'\n{patch_text}\nEOF\n"); diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 44af72c746..2c05dba87f 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -18,12 +18,23 @@ use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; +pub(crate) enum InternalApplyPatchInvocation { + Output(ResponseInputItem), + DelegateToExec(ApplyPatchAction), +} + +impl From for InternalApplyPatchInvocation { + fn from(item: ResponseInputItem) -> Self { + InternalApplyPatchInvocation::Output(item) + } +} + pub(crate) async fn apply_patch( sess: &Session, - sub_id: String, - call_id: String, + sub_id: &str, + call_id: &str, action: ApplyPatchAction, -) -> ResponseInputItem { +) -> InternalApplyPatchInvocation { let writable_roots_snapshot = { #[allow(clippy::unwrap_used)] let guard = sess.writable_roots.lock().unwrap(); @@ -36,34 +47,38 @@ pub(crate) async fn apply_patch( &writable_roots_snapshot, &sess.cwd, ) { - SafetyCheck::AutoApprove { .. } => true, + SafetyCheck::AutoApprove { .. } => { + return InternalApplyPatchInvocation::DelegateToExec(action); + } SafetyCheck::AskUser => { // Compute a readable summary of path changes to include in the // approval request so the user can make an informed decision. let rx_approve = sess - .request_patch_approval(sub_id.clone(), call_id.clone(), &action, None, None) + .request_patch_approval(sub_id.to_owned(), call_id.to_owned(), &action, None, None) .await; match rx_approve.await.unwrap_or_default() { ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, ReviewDecision::Denied | ReviewDecision::Abort => { return ResponseInputItem::FunctionCallOutput { - call_id, + call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: "patch rejected by user".to_string(), success: Some(false), }, - }; + } + .into(); } } } SafetyCheck::Reject { reason } => { return ResponseInputItem::FunctionCallOutput { - call_id, + call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: format!("patch rejected: {reason}"), success: Some(false), }, - }; + } + .into(); } }; @@ -83,8 +98,8 @@ pub(crate) async fn apply_patch( let rx = sess .request_patch_approval( - sub_id.clone(), - call_id.clone(), + sub_id.to_owned(), + call_id.to_owned(), &action, reason.clone(), Some(root.clone()), @@ -96,12 +111,13 @@ pub(crate) async fn apply_patch( ReviewDecision::Approved | ReviewDecision::ApprovedForSession ) { return ResponseInputItem::FunctionCallOutput { - call_id, + call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: "patch rejected by user".to_string(), success: Some(false), }, - }; + } + .into(); } // user approved, extend writable roots for this session @@ -112,9 +128,9 @@ pub(crate) async fn apply_patch( let _ = sess .tx_event .send(Event { - id: sub_id.clone(), + id: sub_id.to_owned(), msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { - call_id: call_id.clone(), + call_id: call_id.to_owned(), auto_approved, changes: convert_apply_patch_to_protocol(&action), }), @@ -173,8 +189,8 @@ pub(crate) async fn apply_patch( )); let rx = sess .request_patch_approval( - sub_id.clone(), - call_id.clone(), + sub_id.to_owned(), + call_id.to_owned(), &action, reason.clone(), Some(root.clone()), @@ -204,9 +220,9 @@ pub(crate) async fn apply_patch( let _ = sess .tx_event .send(Event { - id: sub_id.clone(), + id: sub_id.to_owned(), msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { - call_id: call_id.clone(), + call_id: call_id.to_owned(), stdout: String::from_utf8_lossy(&stdout).to_string(), stderr: String::from_utf8_lossy(&stderr).to_string(), success: success_flag, @@ -214,22 +230,23 @@ pub(crate) async fn apply_patch( }) .await; - match result { + let item = match result { Ok(_) => ResponseInputItem::FunctionCallOutput { - call_id, + call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: String::from_utf8_lossy(&stdout).to_string(), success: None, }, }, Err(e) => ResponseInputItem::FunctionCallOutput { - call_id, + call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: format!("error: {e:#}, stderr: {}", String::from_utf8_lossy(&stderr)), success: Some(false), }, }, - } + }; + InternalApplyPatchInvocation::Output(item) } /// Return the first path in `hunks` that is NOT under any of the diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 3ab3e8d780..c9eabc5d13 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -29,6 +29,7 @@ use tracing::trace; use tracing::warn; use uuid::Uuid; +use crate::apply_patch::InternalApplyPatchInvocation; use crate::apply_patch::convert_apply_patch_to_protocol; use crate::apply_patch::get_writable_roots; use crate::apply_patch::{self}; @@ -1411,82 +1412,118 @@ async fn handle_container_exec_with_params( call_id: String, ) -> ResponseInputItem { // check if this was a patch, and apply it if so - match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { - MaybeApplyPatchVerified::Body(changes) => { - return apply_patch::apply_patch(sess, sub_id, call_id, changes).await; - } - MaybeApplyPatchVerified::CorrectnessError(parse_error) => { - // It looks like an invocation of `apply_patch`, but we - // could not resolve it into a patch that would apply - // cleanly. Return to model for resample. - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("error: {parse_error:#}"), - success: None, - }, - }; - } - MaybeApplyPatchVerified::ShellParseError(error) => { - trace!("Failed to parse shell command, {error:?}"); - } - MaybeApplyPatchVerified::NotApplyPatch => (), - } - - // safety checks - let safety = { - let state = sess.state.lock().unwrap(); - assess_command_safety( - ¶ms.command, - sess.approval_policy, - &sess.sandbox_policy, - &state.approved_commands, - ) - }; - let sandbox_type = match safety { - SafetyCheck::AutoApprove { sandbox_type } => sandbox_type, - SafetyCheck::AskUser => { - let rx_approve = sess - .request_command_approval( - sub_id.clone(), - call_id.clone(), - params.command.clone(), - params.cwd.clone(), - None, - ) - .await; - match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved => (), - ReviewDecision::ApprovedForSession => { - sess.add_approved_command(params.command.clone()); + let apply_patch_action_for_exec = + match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { + MaybeApplyPatchVerified::Body(changes) => { + match apply_patch::apply_patch(sess, &sub_id, &call_id, changes).await { + InternalApplyPatchInvocation::Output(item) => return item, + InternalApplyPatchInvocation::DelegateToExec(action) => Some(action), } - ReviewDecision::Denied | ReviewDecision::Abort => { + } + MaybeApplyPatchVerified::CorrectnessError(parse_error) => { + // It looks like an invocation of `apply_patch`, but we + // could not resolve it into a patch that would apply + // cleanly. Return to model for resample. + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!("error: {parse_error:#}"), + success: None, + }, + }; + } + MaybeApplyPatchVerified::ShellParseError(error) => { + trace!("Failed to parse shell command, {error:?}"); + None + } + MaybeApplyPatchVerified::NotApplyPatch => None, + }; + + let (sandbox_type, params) = match apply_patch_action_for_exec { + Some(ApplyPatchAction { patch, cwd, .. }) => { + // If we are applying a patch, we do not run the command in a sandbox. + // Instead, we run it directly in the host environment. + ( + // TODO(mbolin): Need to get this from assess_command_safety()? + SandboxType::None, + ExecParams { + // TODO(mbolin): Do not blow up if current_exe is not UTF-8? + #[allow(clippy::unwrap_used)] + command: vec![ + std::env::current_exe() + .ok() + .unwrap() + .to_string_lossy() + .to_string(), + "--codex-run-as-apply-patch".to_string(), + patch, + ], + cwd, + timeout_ms: params.timeout_ms, + env: HashMap::new(), + }, + ) + } + None => { + // safety checks + let safety = { + let state = sess.state.lock().unwrap(); + assess_command_safety( + ¶ms.command, + sess.approval_policy, + &sess.sandbox_policy, + &state.approved_commands, + ) + }; + let sandbox_type = match safety { + SafetyCheck::AutoApprove { sandbox_type } => sandbox_type, + SafetyCheck::AskUser => { + let rx_approve = sess + .request_command_approval( + sub_id.clone(), + call_id.clone(), + params.command.clone(), + params.cwd.clone(), + None, + ) + .await; + match rx_approve.await.unwrap_or_default() { + ReviewDecision::Approved => (), + ReviewDecision::ApprovedForSession => { + sess.add_approved_command(params.command.clone()); + } + ReviewDecision::Denied | ReviewDecision::Abort => { + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: "exec command rejected by user".to_string(), + success: None, + }, + }; + } + } + // No sandboxing is applied because the user has given + // explicit approval. Often, we end up in this case because + // the command cannot be run in a sandbox, such as + // installing a new dependency that requires network access. + SandboxType::None + } + SafetyCheck::Reject { reason } => { return ResponseInputItem::FunctionCallOutput { call_id, output: FunctionCallOutputPayload { - content: "exec command rejected by user".to_string(), + content: format!("exec command rejected: {reason}"), success: None, }, }; } - } - // No sandboxing is applied because the user has given - // explicit approval. Often, we end up in this case because - // the command cannot be run in a sandbox, such as - // installing a new dependency that requires network access. - SandboxType::None - } - SafetyCheck::Reject { reason } => { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("exec command rejected: {reason}"), - success: None, - }, }; + + (sandbox_type, params) } }; + // This will look funny for apply_patch? sess.notify_exec_command_begin(&sub_id, &call_id, ¶ms) .await;