codex: move apply_patch onto environment filesystem

This commit is contained in:
starr-openai
2026-04-06 16:40:53 -07:00
parent 0a346655bd
commit 59da18ca84
5 changed files with 569 additions and 14 deletions
+100 -11
View File
@@ -1,7 +1,9 @@
use std::collections::HashMap;
use std::path::Path;
use std::path::PathBuf;
use std::sync::LazyLock;
use similar::TextDiff;
use tree_sitter::Parser;
use tree_sitter::Query;
use tree_sitter::QueryCursor;
@@ -12,9 +14,11 @@ use crate::ApplyPatchAction;
use crate::ApplyPatchArgs;
use crate::ApplyPatchError;
use crate::ApplyPatchFileChange;
use crate::ApplyPatchFileSystem;
use crate::ApplyPatchFileUpdate;
use crate::IoError;
use crate::MaybeApplyPatchVerified;
use crate::derive_new_contents_from_chunks_with_fs;
use crate::parser::Hunk;
use crate::parser::ParseError;
use crate::parser::parse_patch;
@@ -149,17 +153,7 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp
hunks,
workdir,
}) => {
let effective_cwd = workdir
.as_ref()
.map(|dir| {
let path = Path::new(dir);
if path.is_absolute() {
path.to_path_buf()
} else {
cwd.join(path)
}
})
.unwrap_or_else(|| cwd.to_path_buf());
let effective_cwd = resolve_effective_cwd(cwd, workdir.as_deref());
let mut changes = HashMap::new();
for hunk in hunks {
let path = hunk.resolve_path(&effective_cwd);
@@ -216,6 +210,101 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp
}
}
/// Async variant of [`maybe_parse_apply_patch_verified`] that reads file
/// contents through an abstract filesystem instead of directly from `std::fs`.
pub async fn maybe_parse_apply_patch_verified_with_fs(
argv: &[String],
cwd: &Path,
fs: &dyn ApplyPatchFileSystem,
) -> MaybeApplyPatchVerified {
if let [body] = argv
&& parse_patch(body).is_ok()
{
return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation);
}
if let Some((_, script)) = parse_shell_script(argv)
&& parse_patch(script).is_ok()
{
return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation);
}
match maybe_parse_apply_patch(argv) {
MaybeApplyPatch::Body(ApplyPatchArgs {
patch,
hunks,
workdir,
}) => {
let effective_cwd = resolve_effective_cwd(cwd, workdir.as_deref());
let mut changes = HashMap::new();
for hunk in hunks {
let path = hunk.resolve_path(&effective_cwd);
match hunk {
Hunk::AddFile { contents, .. } => {
changes.insert(path, ApplyPatchFileChange::Add { content: contents });
}
Hunk::DeleteFile { .. } => {
let content = match fs.read_text(&path).await {
Ok(content) => content,
Err(err) => {
return MaybeApplyPatchVerified::CorrectnessError(err);
}
};
changes.insert(path, ApplyPatchFileChange::Delete { content });
}
Hunk::UpdateFile {
move_path, chunks, ..
} => {
let applied =
match derive_new_contents_from_chunks_with_fs(&path, &chunks, fs).await
{
Ok(applied) => applied,
Err(err) => {
return MaybeApplyPatchVerified::CorrectnessError(err);
}
};
let unified_diff =
TextDiff::from_lines(&applied.original_contents, &applied.new_contents)
.unified_diff()
.context_radius(1)
.to_string();
changes.insert(
path,
ApplyPatchFileChange::Update {
unified_diff,
move_path: move_path.map(|p| effective_cwd.join(p)),
new_content: applied.new_contents,
},
);
}
}
}
MaybeApplyPatchVerified::Body(ApplyPatchAction {
changes,
patch,
cwd: effective_cwd,
})
}
MaybeApplyPatch::ShellParseError(err) => MaybeApplyPatchVerified::ShellParseError(err),
MaybeApplyPatch::PatchParseError(err) => {
MaybeApplyPatchVerified::CorrectnessError(err.into())
}
MaybeApplyPatch::NotApplyPatch => MaybeApplyPatchVerified::NotApplyPatch,
}
}
fn resolve_effective_cwd(cwd: &Path, workdir: Option<&str>) -> PathBuf {
workdir
.map(Path::new)
.map(|path| {
if path.is_absolute() {
path.to_path_buf()
} else {
cwd.join(path)
}
})
.unwrap_or_else(|| cwd.to_path_buf())
}
/// Extract the heredoc body (and optional `cd` workdir) from a `bash -lc` script
/// that invokes the apply_patch tool using a heredoc.
///
+315 -1
View File
@@ -4,8 +4,10 @@ mod seek_sequence;
mod standalone_executable;
use std::collections::HashMap;
use std::future::Future;
use std::path::Path;
use std::path::PathBuf;
use std::pin::Pin;
use anyhow::Context;
use anyhow::Result;
@@ -18,6 +20,7 @@ use similar::TextDiff;
use thiserror::Error;
pub use invocation::maybe_parse_apply_patch_verified;
pub use invocation::maybe_parse_apply_patch_verified_with_fs;
pub use standalone_executable::main;
use crate::invocation::ExtractHeredocError;
@@ -50,6 +53,15 @@ pub enum ApplyPatchError {
ImplicitInvocation,
}
impl ApplyPatchError {
pub fn io_error(context: impl Into<String>, source: std::io::Error) -> Self {
Self::IoError(IoError {
context: context.into(),
source,
})
}
}
impl From<std::io::Error> for ApplyPatchError {
fn from(err: std::io::Error) -> Self {
ApplyPatchError::IoError(IoError {
@@ -179,6 +191,33 @@ impl ApplyPatchAction {
}
}
/// Filesystem operations required to verify and apply a patch.
///
/// This keeps the patch parser/diff logic independent from the host filesystem
/// so callers can provide sandboxed or remote-backed implementations.
pub trait ApplyPatchFileSystem: Send + Sync {
fn read_text<'a>(
&'a self,
path: &'a Path,
) -> Pin<Box<dyn Future<Output = std::result::Result<String, ApplyPatchError>> + Send + 'a>>;
fn write_text<'a>(
&'a self,
path: &'a Path,
contents: String,
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>>;
fn create_dir_all<'a>(
&'a self,
path: &'a Path,
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>>;
fn remove_file<'a>(
&'a self,
path: &'a Path,
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>>;
}
/// Applies the patch and prints the result to stdout/stderr.
pub fn apply_patch(
patch: &str,
@@ -359,6 +398,14 @@ fn derive_new_contents_from_chunks(
}
};
derive_new_contents_from_text(&original_contents, path, chunks)
}
pub(crate) fn derive_new_contents_from_text(
original_contents: &str,
path: &Path,
chunks: &[UpdateFileChunk],
) -> std::result::Result<AppliedPatch, ApplyPatchError> {
let mut original_lines: Vec<String> = original_contents.split('\n').map(String::from).collect();
// Drop the trailing empty element that results from the final newline so
@@ -375,11 +422,88 @@ fn derive_new_contents_from_chunks(
}
let new_contents = new_lines.join("\n");
Ok(AppliedPatch {
original_contents,
original_contents: original_contents.to_string(),
new_contents,
})
}
pub(crate) async fn derive_new_contents_from_chunks_with_fs(
path: &Path,
chunks: &[UpdateFileChunk],
fs: &dyn ApplyPatchFileSystem,
) -> std::result::Result<AppliedPatch, ApplyPatchError> {
let original_contents = match fs.read_text(path).await {
Ok(contents) => contents,
Err(ApplyPatchError::IoError(IoError { source, .. })) => {
return Err(ApplyPatchError::IoError(IoError {
context: format!("Failed to read file to update {}", path.display()),
source,
}));
}
Err(err) => return Err(err),
};
derive_new_contents_from_text(&original_contents, path, chunks)
}
/// Apply a verified patch action through an abstract filesystem.
pub async fn apply_action_with_fs(
action: &ApplyPatchAction,
fs: &dyn ApplyPatchFileSystem,
) -> std::result::Result<AffectedPaths, ApplyPatchError> {
if action.is_empty() {
return Err(ApplyPatchError::IoError(IoError {
context: "No files were modified.".to_string(),
source: std::io::Error::other("empty patch"),
}));
}
let mut added = Vec::new();
let mut modified = Vec::new();
let mut deleted = Vec::new();
for (path, change) in action.changes() {
match change {
ApplyPatchFileChange::Add { content } => {
if let Some(parent) = path.parent()
&& !parent.as_os_str().is_empty()
{
fs.create_dir_all(parent).await?;
}
fs.write_text(path, content.clone()).await?;
added.push(path.clone());
}
ApplyPatchFileChange::Delete { .. } => {
fs.remove_file(path).await?;
deleted.push(path.clone());
}
ApplyPatchFileChange::Update {
new_content,
move_path,
..
} => {
if let Some(dest) = move_path {
if let Some(parent) = dest.parent()
&& !parent.as_os_str().is_empty()
{
fs.create_dir_all(parent).await?;
}
fs.write_text(dest, new_content.clone()).await?;
fs.remove_file(path).await?;
modified.push(dest.clone());
} else {
fs.write_text(path, new_content.clone()).await?;
modified.push(path.clone());
}
}
}
}
Ok(AffectedPaths {
added,
modified,
deleted,
})
}
/// Compute a list of replacements needed to transform `original_lines` into the
/// new lines, given the patch `chunks`. Each replacement is returned as
/// `(start_index, old_len, new_lines)`.
@@ -556,9 +680,120 @@ mod tests {
use super::*;
use pretty_assertions::assert_eq;
use std::fs;
use std::future::Future;
use std::pin::Pin;
use std::string::ToString;
use std::sync::Arc;
use std::task::Context;
use std::task::Poll;
use std::task::Wake;
use std::task::Waker;
use tempfile::tempdir;
#[derive(Default)]
struct NoopWake;
impl Wake for NoopWake {
fn wake(self: Arc<Self>) {}
}
fn block_on<F: Future>(future: F) -> F::Output {
let waker = Waker::from(Arc::new(NoopWake));
let mut context = Context::from_waker(&waker);
let mut future = Pin::from(Box::new(future));
loop {
match future.as_mut().poll(&mut context) {
Poll::Ready(output) => return output,
Poll::Pending => std::thread::yield_now(),
}
}
}
#[derive(Clone, Default)]
struct TestApplyPatchFileSystem {
files: std::sync::Arc<std::sync::Mutex<HashMap<PathBuf, String>>>,
removed: std::sync::Arc<std::sync::Mutex<Vec<PathBuf>>>,
directories: std::sync::Arc<std::sync::Mutex<Vec<PathBuf>>>,
}
impl TestApplyPatchFileSystem {
fn with_file(path: &Path, content: &str) -> Self {
let file_system = Self::default();
file_system
.files
.lock()
.expect("lock files")
.insert(path.to_path_buf(), content.to_string());
file_system
}
}
impl ApplyPatchFileSystem for TestApplyPatchFileSystem {
fn read_text<'a>(
&'a self,
path: &'a Path,
) -> Pin<Box<dyn Future<Output = std::result::Result<String, ApplyPatchError>> + Send + 'a>>
{
Box::pin(async move {
self.files
.lock()
.expect("lock files")
.get(path)
.cloned()
.ok_or_else(|| {
ApplyPatchError::io_error(
format!("missing test file {}", path.display()),
std::io::Error::new(std::io::ErrorKind::NotFound, "missing"),
)
})
})
}
fn write_text<'a>(
&'a self,
path: &'a Path,
contents: String,
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>>
{
Box::pin(async move {
self.files
.lock()
.expect("lock files")
.insert(path.to_path_buf(), contents);
Ok(())
})
}
fn create_dir_all<'a>(
&'a self,
path: &'a Path,
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>>
{
Box::pin(async move {
self.directories
.lock()
.expect("lock dirs")
.push(path.to_path_buf());
Ok(())
})
}
fn remove_file<'a>(
&'a self,
path: &'a Path,
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>>
{
Box::pin(async move {
self.files.lock().expect("lock files").remove(path);
self.removed
.lock()
.expect("lock removed")
.push(path.to_path_buf());
Ok(())
})
}
}
/// Helper to construct a patch with the given body.
fn wrap_patch(body: &str) -> String {
format!("*** Begin Patch\n{body}\n*** End Patch")
@@ -1071,4 +1306,83 @@ g
let result = apply_patch(&patch, &mut stdout, &mut stderr);
assert!(result.is_err());
}
#[test]
fn apply_action_with_fs_updates_file_without_touching_host_fs() {
let path = PathBuf::from("/virtual/update.txt");
let patch = wrap_patch(&format!(
r#"*** Update File: {}
@@
-before
+after"#,
path.display()
));
let argv = vec!["apply_patch".to_string(), patch];
let fs = TestApplyPatchFileSystem::with_file(&path, "before\n");
let action = match block_on(maybe_parse_apply_patch_verified_with_fs(
&argv,
Path::new("/"),
&fs,
)) {
MaybeApplyPatchVerified::Body(action) => action,
other => panic!("expected patch body, got {other:?}"),
};
let affected = block_on(apply_action_with_fs(&action, &fs)).expect("apply action");
assert_eq!(affected.modified, vec![path.clone()]);
assert_eq!(
fs.files.lock().expect("lock files").get(&path).cloned(),
Some("after\n".to_string())
);
}
#[test]
fn maybe_parse_apply_patch_verified_with_fs_uses_abstract_fs_for_update_and_delete() {
let update_path = PathBuf::from("/virtual/update.txt");
let delete_path = PathBuf::from("/virtual/delete.txt");
let patch = wrap_patch(&format!(
r#"*** Update File: {}
@@
-before
+after
*** Delete File: {}"#,
update_path.display(),
delete_path.display()
));
let argv = vec!["apply_patch".to_string(), patch];
let fs = TestApplyPatchFileSystem::default();
fs.files
.lock()
.expect("lock files")
.insert(update_path.clone(), "before\n".to_string());
fs.files
.lock()
.expect("lock files")
.insert(delete_path.clone(), "gone\n".to_string());
let action = match block_on(maybe_parse_apply_patch_verified_with_fs(
&argv,
Path::new("/"),
&fs,
)) {
MaybeApplyPatchVerified::Body(action) => action,
other => panic!("expected patch body, got {other:?}"),
};
assert_eq!(
action.changes().get(&delete_path),
Some(&ApplyPatchFileChange::Delete {
content: "gone\n".to_string()
})
);
assert_eq!(
action.changes().get(&update_path),
Some(&ApplyPatchFileChange::Update {
unified_diff: "@@ -1 +1 @@\n-before\n+after\n".to_string(),
move_path: None,
new_content: "after\n".to_string(),
})
);
}
}
+111
View File
@@ -4,11 +4,21 @@ use crate::safety::SafetyCheck;
use crate::safety::assess_patch_safety;
use crate::tools::sandboxing::ExecApprovalRequirement;
use codex_apply_patch::ApplyPatchAction;
use codex_apply_patch::ApplyPatchError;
use codex_apply_patch::ApplyPatchFileChange;
use codex_apply_patch::ApplyPatchFileSystem;
use codex_exec_server::CreateDirectoryOptions;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::RemoveOptions;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::FileSystemSandboxPolicy;
use codex_utils_absolute_path::AbsolutePathBuf;
use std::collections::HashMap;
use std::future::Future;
use std::io;
use std::path::PathBuf;
use std::pin::Pin;
use std::sync::Arc;
pub(crate) enum InternalApplyPatchInvocation {
/// The `apply_patch` call was handled programmatically, without any sort
@@ -33,6 +43,107 @@ pub(crate) struct ApplyPatchExec {
pub(crate) exec_approval_requirement: ExecApprovalRequirement,
}
pub(crate) struct EnvironmentApplyPatchFileSystem {
file_system: Arc<dyn ExecutorFileSystem>,
}
impl EnvironmentApplyPatchFileSystem {
pub(crate) fn new(file_system: Arc<dyn ExecutorFileSystem>) -> Self {
Self { file_system }
}
}
impl ApplyPatchFileSystem for EnvironmentApplyPatchFileSystem {
fn read_text<'a>(
&'a self,
path: &'a std::path::Path,
) -> Pin<Box<dyn Future<Output = std::result::Result<String, ApplyPatchError>> + Send + 'a>>
{
Box::pin(async move {
let path = absolute_path(path)?;
let bytes = self.file_system.read_file(&path).await.map_err(|source| {
ApplyPatchError::io_error(format!("Failed to read {}", path.display()), source)
})?;
String::from_utf8(bytes).map_err(|source| {
ApplyPatchError::io_error(
format!("Failed to decode UTF-8 for {}", path.display()),
io::Error::new(io::ErrorKind::InvalidData, source.to_string()),
)
})
})
}
fn write_text<'a>(
&'a self,
path: &'a std::path::Path,
contents: String,
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>> {
Box::pin(async move {
let path = absolute_path(path)?;
self.file_system
.write_file(&path, contents.into_bytes())
.await
.map_err(|source| {
ApplyPatchError::io_error(
format!("Failed to write file {}", path.display()),
source,
)
})
})
}
fn create_dir_all<'a>(
&'a self,
path: &'a std::path::Path,
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>> {
Box::pin(async move {
let path = absolute_path(path)?;
self.file_system
.create_directory(&path, CreateDirectoryOptions { recursive: true })
.await
.map_err(|source| {
ApplyPatchError::io_error(
format!("Failed to create parent directories for {}", path.display()),
source,
)
})
})
}
fn remove_file<'a>(
&'a self,
path: &'a std::path::Path,
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>> {
Box::pin(async move {
let path = absolute_path(path)?;
self.file_system
.remove(
&path,
RemoveOptions {
recursive: false,
force: false,
},
)
.await
.map_err(|source| {
ApplyPatchError::io_error(
format!("Failed to delete file {}", path.display()),
source,
)
})
})
}
}
fn absolute_path(path: &std::path::Path) -> std::result::Result<AbsolutePathBuf, ApplyPatchError> {
AbsolutePathBuf::from_absolute_path(path).map_err(|error| {
ApplyPatchError::io_error(
format!("Expected absolute path for apply_patch: {}", path.display()),
io::Error::new(io::ErrorKind::InvalidInput, error.to_string()),
)
})
}
pub(crate) async fn apply_patch(
turn_context: &TurnContext,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
@@ -1,6 +1,7 @@
use std::path::Path;
use crate::apply_patch;
use crate::apply_patch::EnvironmentApplyPatchFileSystem;
use crate::apply_patch::InternalApplyPatchInvocation;
use crate::apply_patch::convert_apply_patch_to_protocol;
use crate::codex::Session;
@@ -167,7 +168,7 @@ impl ToolHandler for ApplyPatchHandler {
// Avoid building temporary ExecParams/command vectors; derive directly from inputs.
let cwd = turn.cwd.clone();
let command = vec!["apply_patch".to_string(), patch_input.clone()];
match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd) {
match parse_apply_patch_verified(turn.as_ref(), &command, &cwd).await {
codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => {
let (file_paths, effective_additional_permissions, file_system_sandbox_policy) =
effective_patch_permissions(session.as_ref(), turn.as_ref(), &changes).await;
@@ -262,7 +263,7 @@ pub(crate) async fn intercept_apply_patch(
call_id: &str,
tool_name: &str,
) -> Result<Option<FunctionToolOutput>, FunctionCallError> {
match codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd) {
match parse_apply_patch_verified(turn.as_ref(), command, cwd).await {
codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => {
session
.record_model_warning(
@@ -346,6 +347,15 @@ pub(crate) async fn intercept_apply_patch(
}
}
async fn parse_apply_patch_verified(
turn: &TurnContext,
command: &[String],
cwd: &Path,
) -> codex_apply_patch::MaybeApplyPatchVerified {
let fs = EnvironmentApplyPatchFileSystem::new(turn.environment.get_filesystem());
codex_apply_patch::maybe_parse_apply_patch_verified_with_fs(command, cwd, &fs).await
}
#[cfg(test)]
#[path = "apply_patch_tests.rs"]
mod tests;
@@ -4,6 +4,7 @@
//! decision to avoid re-prompting, builds the self-invocation command for
//! `codex --codex-run-as-apply-patch`, and runs under the current
//! `SandboxAttempt` with a minimal environment.
use crate::apply_patch::EnvironmentApplyPatchFileSystem;
use crate::exec::ExecCapturePolicy;
use crate::guardian::GuardianApprovalRequest;
use crate::guardian::review_approval_request;
@@ -22,6 +23,7 @@ use crate::tools::sandboxing::with_cached_approval;
use codex_apply_patch::ApplyPatchAction;
use codex_apply_patch::CODEX_CORE_APPLY_PATCH_ARG1;
use codex_protocol::exec_output::ExecToolCallOutput;
use codex_protocol::exec_output::StreamOutput;
use codex_protocol::models::PermissionProfile;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::FileChange;
@@ -32,6 +34,7 @@ use codex_utils_absolute_path::AbsolutePathBuf;
use futures::future::BoxFuture;
use std::collections::HashMap;
use std::path::PathBuf;
use std::time::Duration;
#[derive(Debug)]
pub struct ApplyPatchRequest {
@@ -115,6 +118,29 @@ impl ApplyPatchRuntime {
tx_event: ctx.session.get_tx_event(),
})
}
async fn run_with_environment_fs(
req: &ApplyPatchRequest,
fs: EnvironmentApplyPatchFileSystem,
) -> Result<ExecToolCallOutput, ToolError> {
let affected = codex_apply_patch::apply_action_with_fs(&req.action, &fs)
.await
.map_err(|err| ToolError::Rejected(err.to_string()))?;
let mut stdout = Vec::new();
codex_apply_patch::print_summary(&affected, &mut stdout)
.map_err(|err| ToolError::Rejected(err.to_string()))?;
let stdout = String::from_utf8(stdout).map_err(|err| {
ToolError::Rejected(format!("apply_patch wrote non-UTF-8 output: {err}"))
})?;
Ok(ExecToolCallOutput {
exit_code: 0,
stdout: StreamOutput::new(stdout.clone()),
stderr: StreamOutput::new(String::new()),
aggregated_output: StreamOutput::new(stdout),
duration: Duration::ZERO,
timed_out: false,
})
}
}
impl Sandboxable for ApplyPatchRuntime {
@@ -211,6 +237,11 @@ impl ToolRuntime<ApplyPatchRequest, ExecToolCallOutput> for ApplyPatchRuntime {
attempt: &SandboxAttempt<'_>,
ctx: &ToolCtx,
) -> Result<ExecToolCallOutput, ToolError> {
if ctx.turn.environment.exec_server_url().is_some() {
let fs = EnvironmentApplyPatchFileSystem::new(ctx.turn.environment.get_filesystem());
return Self::run_with_environment_fs(req, fs).await;
}
#[cfg(target_os = "windows")]
let command = Self::build_sandbox_command(req, &ctx.turn.config.codex_home)?;
#[cfg(not(target_os = "windows"))]