openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-16 12:13:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: run apply_patch calls through the sandbox

Browse Source
This commit is contained in:
Michael Bolin
2025-07-28 08:53:04 -07:00
parent 1b1d4b69a6
commit 73a21d0a10
4 changed files with 226 additions and 118 deletions
Download Patch File Download Diff File Expand all files Collapse all files

62
codex-rs/apply-patch/src/lib.rs
View File

@@ -58,16 +58,22 @@ impl PartialEq for IoError {
#[derive(Debug, PartialEq)]
pub enum MaybeApplyPatch {
Body(Vec<Hunk>),
Body(ApplyPatchSource),
ShellParseError(ExtractHeredocError),
PatchParseError(ParseError),
NotApplyPatch,
}
#[derive(Debug, PartialEq)]
pub struct ApplyPatchSource {
pub hunks: Vec<Hunk>,
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<PathBuf, ApplyPatchFileChange>,
/// 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(),
})
);
}

54
codex-rs/apply-patch/src/parser.rs
View File

@@ -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<Vec<Hunk>, ParseError> {
pub fn parse_patch(patch: &str) -> Result<ApplyPatchSource, ParseError> {
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<Vec<Hunk>, ParseError> {
fn parse_patch_text(patch: &str, mode: ParseMode) -> Result<ApplyPatchSource, ParseError> {
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<Vec<Hunk>, 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");

63
codex-rs/core/src/apply_patch.rs
View File

@@ -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<ResponseInputItem> 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

165
codex-rs/core/src/codex.rs
View File

@@ -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(&params.command, &params.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(
&params.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(&params.command, &params.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(
&params.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, &params)
.await;