diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index f116c790..dc11aed0 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -1,19 +1,12 @@ use crate::codex::Session; use crate::models::FunctionCallOutputPayload; use crate::models::ResponseInputItem; -use crate::protocol::Event; -use crate::protocol::EventMsg; use crate::protocol::FileChange; -use crate::protocol::PatchApplyBeginEvent; -use crate::protocol::PatchApplyEndEvent; use crate::protocol::ReviewDecision; use crate::safety::SafetyCheck; use crate::safety::assess_patch_safety; -use anyhow::Context; -use codex_apply_patch::AffectedPaths; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; -use codex_apply_patch::print_summary; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; @@ -26,12 +19,18 @@ pub(crate) enum InternalApplyPatchInvocation { /// result to use with the `shell` function call that contained `apply_patch`. Output(ResponseInputItem), - /// The `apply_patch` call was auto-approved, which means that, on the - /// surface, it appears to be safe, but it should be run in a sandbox if the - /// user has configured one because a path being written could be a hard - /// link to a file outside the writable folders, so only the sandbox can - /// faithfully prevent the write in that case. - DelegateToExec(ApplyPatchAction), + /// The `apply_patch` call was approved, either automatically because it + /// appears that it should be allowed based on the user's sandbox policy + /// *or* because the user explicitly approved it. In either case, we use + /// exec with [`CODEX_APPLY_PATCH_ARG1`] to realize the `apply_patch` call, + /// but [`ApplyPatchExec::auto_approved`] is used to determine the sandbox + /// used with the `exec()`. + DelegateToExec(ApplyPatchExec), +} + +pub(crate) struct ApplyPatchExec { + pub(crate) action: ApplyPatchAction, + pub(crate) user_explicitly_approved_this_action: bool, } impl From for InternalApplyPatchInvocation { @@ -52,254 +51,57 @@ pub(crate) async fn apply_patch( guard.clone() }; - let auto_approved = match assess_patch_safety( + match assess_patch_safety( &action, sess.approval_policy, &writable_roots_snapshot, &sess.cwd, ) { SafetyCheck::AutoApprove { .. } => { - return InternalApplyPatchInvocation::DelegateToExec(action); + InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { + action, + user_explicitly_approved_this_action: false, + }) } SafetyCheck::AskUser => { // Compute a readable summary of path changes to include in the // approval request so the user can make an informed decision. + // + // Note that it might be worth expanding this approval request to + // give the user the option to expand the set of writable roots so + // that similar patches can be auto-approved in the future during + // this session. let rx_approve = sess .request_patch_approval(sub_id.to_owned(), call_id.to_owned(), &action, None, None) .await; match rx_approve.await.unwrap_or_default() { - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, + ReviewDecision::Approved | ReviewDecision::ApprovedForSession => { + InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { + action, + user_explicitly_approved_this_action: true, + }) + } ReviewDecision::Denied | ReviewDecision::Abort => { - return ResponseInputItem::FunctionCallOutput { + ResponseInputItem::FunctionCallOutput { call_id: call_id.to_owned(), output: FunctionCallOutputPayload { content: "patch rejected by user".to_string(), success: Some(false), }, } - .into(); + .into() } } } - SafetyCheck::Reject { reason } => { - return ResponseInputItem::FunctionCallOutput { - call_id: call_id.to_owned(), - output: FunctionCallOutputPayload { - content: format!("patch rejected: {reason}"), - success: Some(false), - }, - } - .into(); - } - }; - - // Verify write permissions before touching the filesystem. - let writable_snapshot = { - #[allow(clippy::unwrap_used)] - sess.writable_roots.lock().unwrap().clone() - }; - - if let Some(offending) = first_offending_path(&action, &writable_snapshot, &sess.cwd) { - let root = offending.parent().unwrap_or(&offending).to_path_buf(); - - let reason = Some(format!( - "grant write access to {} for this session", - root.display() - )); - - let rx = sess - .request_patch_approval( - sub_id.to_owned(), - call_id.to_owned(), - &action, - reason.clone(), - Some(root.clone()), - ) - .await; - - if !matches!( - rx.await.unwrap_or_default(), - ReviewDecision::Approved | ReviewDecision::ApprovedForSession - ) { - return ResponseInputItem::FunctionCallOutput { - call_id: call_id.to_owned(), - output: FunctionCallOutputPayload { - content: "patch rejected by user".to_string(), - success: Some(false), - }, - } - .into(); - } - - // user approved, extend writable roots for this session - #[allow(clippy::unwrap_used)] - sess.writable_roots.lock().unwrap().push(root); - } - - let _ = sess - .tx_event - .send(Event { - id: sub_id.to_owned(), - msg: EventMsg::PatchApplyBegin(PatchApplyBeginEvent { - call_id: call_id.to_owned(), - auto_approved, - changes: convert_apply_patch_to_protocol(&action), - }), - }) - .await; - - let mut stdout = Vec::new(); - let mut stderr = Vec::new(); - // Enforce writable roots. If a write is blocked, collect offending root - // and prompt the user to extend permissions. - let mut result = apply_changes_from_apply_patch_and_report(&action, &mut stdout, &mut stderr); - - if let Err(err) = &result { - if err.kind() == std::io::ErrorKind::PermissionDenied { - // Determine first offending path. - let offending_opt = action - .changes() - .iter() - .flat_map(|(path, change)| match change { - ApplyPatchFileChange::Add { .. } => vec![path.as_ref()], - ApplyPatchFileChange::Delete => vec![path.as_ref()], - ApplyPatchFileChange::Update { - move_path: Some(move_path), - .. - } => { - vec![path.as_ref(), move_path.as_ref()] - } - ApplyPatchFileChange::Update { - move_path: None, .. - } => vec![path.as_ref()], - }) - .find_map(|path: &Path| { - // ApplyPatchAction promises to guarantee absolute paths. - if !path.is_absolute() { - panic!("apply_patch invariant failed: path is not absolute: {path:?}"); - } - - let writable = { - #[allow(clippy::unwrap_used)] - let roots = sess.writable_roots.lock().unwrap(); - roots.iter().any(|root| path.starts_with(root)) - }; - if writable { - None - } else { - Some(path.to_path_buf()) - } - }); - - if let Some(offending) = offending_opt { - let root = offending.parent().unwrap_or(&offending).to_path_buf(); - - let reason = Some(format!( - "grant write access to {} for this session", - root.display() - )); - let rx = sess - .request_patch_approval( - sub_id.to_owned(), - call_id.to_owned(), - &action, - reason.clone(), - Some(root.clone()), - ) - .await; - if matches!( - rx.await.unwrap_or_default(), - ReviewDecision::Approved | ReviewDecision::ApprovedForSession - ) { - // Extend writable roots. - #[allow(clippy::unwrap_used)] - sess.writable_roots.lock().unwrap().push(root); - stdout.clear(); - stderr.clear(); - result = apply_changes_from_apply_patch_and_report( - &action, - &mut stdout, - &mut stderr, - ); - } - } - } - } - - // Emit PatchApplyEnd event. - let success_flag = result.is_ok(); - let _ = sess - .tx_event - .send(Event { - id: sub_id.to_owned(), - msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { - call_id: call_id.to_owned(), - stdout: String::from_utf8_lossy(&stdout).to_string(), - stderr: String::from_utf8_lossy(&stderr).to_string(), - success: success_flag, - }), - }) - .await; - - let item = match result { - Ok(_) => ResponseInputItem::FunctionCallOutput { + SafetyCheck::Reject { reason } => ResponseInputItem::FunctionCallOutput { call_id: call_id.to_owned(), output: FunctionCallOutputPayload { - content: String::from_utf8_lossy(&stdout).to_string(), - success: None, - }, - }, - Err(e) => ResponseInputItem::FunctionCallOutput { - call_id: call_id.to_owned(), - output: FunctionCallOutputPayload { - content: format!("error: {e:#}, stderr: {}", String::from_utf8_lossy(&stderr)), + content: format!("patch rejected: {reason}"), success: Some(false), }, - }, - }; - InternalApplyPatchInvocation::Output(item) -} - -/// Return the first path in `hunks` that is NOT under any of the -/// `writable_roots` (after normalising). If all paths are acceptable, -/// returns None. -fn first_offending_path( - action: &ApplyPatchAction, - writable_roots: &[PathBuf], - cwd: &Path, -) -> Option { - let changes = action.changes(); - for (path, change) in changes { - let candidate = match change { - ApplyPatchFileChange::Add { .. } => path, - ApplyPatchFileChange::Delete => path, - ApplyPatchFileChange::Update { move_path, .. } => move_path.as_ref().unwrap_or(path), - }; - - let abs = if candidate.is_absolute() { - candidate.clone() - } else { - cwd.join(candidate) - }; - - let mut allowed = false; - for root in writable_roots { - let root_abs = if root.is_absolute() { - root.clone() - } else { - cwd.join(root) - }; - if abs.starts_with(&root_abs) { - allowed = true; - break; - } - } - - if !allowed { - return Some(candidate.clone()); } + .into(), } - None } pub(crate) fn convert_apply_patch_to_protocol( @@ -327,85 +129,6 @@ pub(crate) fn convert_apply_patch_to_protocol( result } -fn apply_changes_from_apply_patch_and_report( - action: &ApplyPatchAction, - stdout: &mut impl std::io::Write, - stderr: &mut impl std::io::Write, -) -> std::io::Result<()> { - match apply_changes_from_apply_patch(action) { - Ok(affected_paths) => { - print_summary(&affected_paths, stdout)?; - } - Err(err) => { - writeln!(stderr, "{err:?}")?; - } - } - - Ok(()) -} - -fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result { - let mut added: Vec = Vec::new(); - let mut modified: Vec = Vec::new(); - let mut deleted: Vec = Vec::new(); - - let changes = action.changes(); - for (path, change) in changes { - match change { - ApplyPatchFileChange::Add { content } => { - if let Some(parent) = path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent).with_context(|| { - format!("Failed to create parent directories for {}", path.display()) - })?; - } - } - std::fs::write(path, content) - .with_context(|| format!("Failed to write file {}", path.display()))?; - added.push(path.clone()); - } - ApplyPatchFileChange::Delete => { - std::fs::remove_file(path) - .with_context(|| format!("Failed to delete file {}", path.display()))?; - deleted.push(path.clone()); - } - ApplyPatchFileChange::Update { - unified_diff: _unified_diff, - move_path, - new_content, - } => { - if let Some(move_path) = move_path { - if let Some(parent) = move_path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent).with_context(|| { - format!( - "Failed to create parent directories for {}", - move_path.display() - ) - })?; - } - } - - std::fs::rename(path, move_path) - .with_context(|| format!("Failed to rename file {}", path.display()))?; - std::fs::write(move_path, new_content)?; - modified.push(move_path.clone()); - deleted.push(path.clone()); - } else { - std::fs::write(path, new_content)?; - modified.push(path.clone()); - } - } - } - } - - Ok(AffectedPaths { - added, - modified, - deleted, - }) -} - pub(crate) fn get_writable_roots(cwd: &Path) -> Vec { let mut writable_roots = Vec::new(); if cfg!(target_os = "macos") { diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index cd92739c..3dd1d513 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4,7 +4,6 @@ use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; -use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; @@ -31,6 +30,7 @@ use tracing::trace; use tracing::warn; use uuid::Uuid; +use crate::apply_patch::ApplyPatchExec; use crate::apply_patch::CODEX_APPLY_PATCH_ARG1; use crate::apply_patch::InternalApplyPatchInvocation; use crate::apply_patch::convert_apply_patch_to_protocol; @@ -74,8 +74,11 @@ use crate::protocol::EventMsg; use crate::protocol::ExecApprovalRequestEvent; use crate::protocol::ExecCommandBeginEvent; use crate::protocol::ExecCommandEndEvent; +use crate::protocol::FileChange; use crate::protocol::InputItem; use crate::protocol::Op; +use crate::protocol::PatchApplyBeginEvent; +use crate::protocol::PatchApplyEndEvent; use crate::protocol::ReviewDecision; use crate::protocol::SandboxPolicy; use crate::protocol::SessionConfiguredEvent; @@ -358,20 +361,32 @@ impl Session { } } - async fn notify_exec_command_begin( - &self, - sub_id: &str, - call_id: &str, - command_for_display: Vec, - command_cwd: &Path, - ) { + async fn notify_exec_command_begin(&self, exec_command_context: ExecCommandContext) { + let ExecCommandContext { + sub_id, + call_id, + command_for_display, + cwd, + apply_patch, + } = exec_command_context; + let msg = match apply_patch { + Some(ApplyPatchCommandContext { + user_explicitly_approved_this_action, + changes, + }) => EventMsg::PatchApplyBegin(PatchApplyBeginEvent { + call_id, + auto_approved: !user_explicitly_approved_this_action, + changes, + }), + None => EventMsg::ExecCommandBegin(ExecCommandBeginEvent { + call_id, + command: command_for_display.clone(), + cwd, + }), + }; let event = Event { id: sub_id.to_string(), - msg: EventMsg::ExecCommandBegin(ExecCommandBeginEvent { - call_id: call_id.to_string(), - command: command_for_display, - cwd: command_cwd.to_path_buf(), - }), + msg, }; let _ = self.tx_event.send(event).await; } @@ -383,18 +398,33 @@ impl Session { stdout: &str, stderr: &str, exit_code: i32, + is_apply_patch: bool, ) { + // Because stdout and stderr could each be up to 100 KiB, we send + // truncated versions. const MAX_STREAM_OUTPUT: usize = 5 * 1024; // 5KiB + let stdout = stdout.chars().take(MAX_STREAM_OUTPUT).collect(); + let stderr = stderr.chars().take(MAX_STREAM_OUTPUT).collect(); + + let msg = if is_apply_patch { + EventMsg::PatchApplyEnd(PatchApplyEndEvent { + call_id: call_id.to_string(), + stdout, + stderr, + success: exit_code == 0, + }) + } else { + EventMsg::ExecCommandEnd(ExecCommandEndEvent { + call_id: call_id.to_string(), + stdout, + stderr, + exit_code, + }) + }; + let event = Event { id: sub_id.to_string(), - // Because stdout and stderr could each be up to 100 KiB, we send - // truncated versions. - msg: EventMsg::ExecCommandEnd(ExecCommandEndEvent { - call_id: call_id.to_string(), - stdout: stdout.chars().take(MAX_STREAM_OUTPUT).collect(), - stderr: stderr.chars().take(MAX_STREAM_OUTPUT).collect(), - exit_code, - }), + msg, }; let _ = self.tx_event.send(event).await; } @@ -502,6 +532,21 @@ impl State { } } +#[derive(Clone, Debug)] +pub(crate) struct ExecCommandContext { + pub(crate) sub_id: String, + pub(crate) call_id: String, + pub(crate) command_for_display: Vec, + pub(crate) cwd: PathBuf, + pub(crate) apply_patch: Option, +} + +#[derive(Clone, Debug)] +pub(crate) struct ApplyPatchCommandContext { + pub(crate) user_explicitly_approved_this_action: bool, + pub(crate) changes: HashMap, +} + /// A series of Turns in response to user input. pub(crate) struct AgentTask { sess: Arc, @@ -1430,35 +1475,39 @@ async fn handle_container_exec_with_params( call_id: String, ) -> ResponseInputItem { // check if this was a patch, and apply it if so - let apply_patch_action_for_exec = - match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { - MaybeApplyPatchVerified::Body(changes) => { - match apply_patch::apply_patch(sess, &sub_id, &call_id, changes).await { - InternalApplyPatchInvocation::Output(item) => return item, - InternalApplyPatchInvocation::DelegateToExec(action) => Some(action), + let apply_patch_exec = match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { + MaybeApplyPatchVerified::Body(changes) => { + match apply_patch::apply_patch(sess, &sub_id, &call_id, changes).await { + InternalApplyPatchInvocation::Output(item) => return item, + InternalApplyPatchInvocation::DelegateToExec(apply_patch_exec) => { + Some(apply_patch_exec) } } - MaybeApplyPatchVerified::CorrectnessError(parse_error) => { - // It looks like an invocation of `apply_patch`, but we - // could not resolve it into a patch that would apply - // cleanly. Return to model for resample. - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: format!("error: {parse_error:#}"), - success: None, - }, - }; - } - MaybeApplyPatchVerified::ShellParseError(error) => { - trace!("Failed to parse shell command, {error:?}"); - None - } - MaybeApplyPatchVerified::NotApplyPatch => None, - }; + } + MaybeApplyPatchVerified::CorrectnessError(parse_error) => { + // It looks like an invocation of `apply_patch`, but we + // could not resolve it into a patch that would apply + // cleanly. Return to model for resample. + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: format!("error: {parse_error:#}"), + success: None, + }, + }; + } + MaybeApplyPatchVerified::ShellParseError(error) => { + trace!("Failed to parse shell command, {error:?}"); + None + } + MaybeApplyPatchVerified::NotApplyPatch => None, + }; - let (params, safety, command_for_display) = match apply_patch_action_for_exec { - Some(ApplyPatchAction { patch, cwd, .. }) => { + let (params, safety, command_for_display) = match &apply_patch_exec { + Some(ApplyPatchExec { + action: ApplyPatchAction { patch, cwd, .. }, + user_explicitly_approved_this_action, + }) => { let path_to_codex = std::env::current_exe() .ok() .map(|p| p.to_string_lossy().to_string()); @@ -1478,13 +1527,22 @@ async fn handle_container_exec_with_params( CODEX_APPLY_PATCH_ARG1.to_string(), patch.clone(), ], - cwd, + cwd: cwd.clone(), timeout_ms: params.timeout_ms, env: HashMap::new(), }; - let safety = - assess_safety_for_untrusted_command(sess.approval_policy, &sess.sandbox_policy); - (params, safety, vec!["apply_patch".to_string(), patch]) + let safety = if *user_explicitly_approved_this_action { + SafetyCheck::AutoApprove { + sandbox_type: SandboxType::None, + } + } else { + assess_safety_for_untrusted_command(sess.approval_policy, &sess.sandbox_policy) + }; + ( + params, + safety, + vec!["apply_patch".to_string(), patch.clone()], + ) } None => { let safety = { @@ -1545,7 +1603,22 @@ async fn handle_container_exec_with_params( } }; - sess.notify_exec_command_begin(&sub_id, &call_id, command_for_display.clone(), ¶ms.cwd) + let exec_command_context = ExecCommandContext { + sub_id: sub_id.clone(), + call_id: call_id.clone(), + command_for_display: command_for_display.clone(), + cwd: params.cwd.clone(), + apply_patch: apply_patch_exec.map( + |ApplyPatchExec { + action, + user_explicitly_approved_this_action, + }| ApplyPatchCommandContext { + user_explicitly_approved_this_action, + changes: convert_apply_patch_to_protocol(&action), + }, + ), + }; + sess.notify_exec_command_begin(exec_command_context.clone()) .await; let params = maybe_run_with_user_profile(params, sess); @@ -1567,8 +1640,15 @@ async fn handle_container_exec_with_params( duration, } = output; - sess.notify_exec_command_end(&sub_id, &call_id, &stdout, &stderr, exit_code) - .await; + sess.notify_exec_command_end( + &sub_id, + &call_id, + &stdout, + &stderr, + exit_code, + exec_command_context.apply_patch.is_some(), + ) + .await; let is_success = exit_code == 0; let content = format_exec_output( @@ -1586,16 +1666,7 @@ async fn handle_container_exec_with_params( } } Err(CodexErr::Sandbox(error)) => { - handle_sandbox_error( - error, - sandbox_type, - params, - command_for_display, - sess, - sub_id, - call_id, - ) - .await + handle_sandbox_error(params, exec_command_context, error, sandbox_type, sess).await } Err(e) => { // Handle non-sandbox errors @@ -1611,14 +1682,17 @@ async fn handle_container_exec_with_params( } async fn handle_sandbox_error( + params: ExecParams, + exec_command_context: ExecCommandContext, error: SandboxErr, sandbox_type: SandboxType, - params: ExecParams, - command_for_display: Vec, sess: &Session, - sub_id: String, - call_id: String, ) -> ResponseInputItem { + let call_id = exec_command_context.call_id.clone(); + let sub_id = exec_command_context.sub_id.clone(); + let cwd = exec_command_context.cwd.clone(); + let is_apply_patch = exec_command_context.apply_patch.is_some(); + // Early out if the user never wants to be asked for approval; just return to the model immediately if sess.approval_policy == AskForApproval::Never { return ResponseInputItem::FunctionCallOutput { @@ -1648,7 +1722,7 @@ async fn handle_sandbox_error( sub_id.clone(), call_id.clone(), params.command.clone(), - params.cwd.clone(), + cwd.clone(), Some("command failed; retry without sandbox?".to_string()), ) .await; @@ -1664,8 +1738,7 @@ async fn handle_sandbox_error( sess.notify_background_event(&sub_id, "retrying command without sandbox") .await; - sess.notify_exec_command_begin(&sub_id, &call_id, command_for_display, ¶ms.cwd) - .await; + sess.notify_exec_command_begin(exec_command_context).await; // This is an escalated retry; the policy will not be // examined and the sandbox has been set to `None`. @@ -1687,8 +1760,15 @@ async fn handle_sandbox_error( duration, } = retry_output; - sess.notify_exec_command_end(&sub_id, &call_id, &stdout, &stderr, exit_code) - .await; + sess.notify_exec_command_end( + &sub_id, + &call_id, + &stdout, + &stderr, + exit_code, + is_apply_patch, + ) + .await; let is_success = exit_code == 0; let content = format_exec_output( diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index f9bc27e0..224705f8 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -41,11 +41,13 @@ pub fn assess_patch_safety( } } - if is_write_patch_constrained_to_writable_paths(action, writable_roots, cwd) { - SafetyCheck::AutoApprove { - sandbox_type: SandboxType::None, - } - } else if policy == AskForApproval::OnFailure { + // Even though the patch *appears* to be constrained to writable paths, it + // is possible that paths in the patch are hard links to files outside the + // writable roots, so we should still run `apply_patch` in a sandbox in that + // case. + if is_write_patch_constrained_to_writable_paths(action, writable_roots, cwd) + || policy == AskForApproval::OnFailure + { // Only auto‑approve when we can actually enforce a sandbox. Otherwise // fall back to asking the user because the patch may touch arbitrary // paths outside the project.