From 68076e61f32e2a3366594bc701df4622d8fa61ea Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Jul 2025 00:04:34 -0700 Subject: [PATCH] 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;