agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

apply-patch: carry paths as PathUri (#28854)

Browse Source 
## Why

Allows the model to edit files that are hosted on a different OS than
where app-server is running.

## What

* Use `PathUri` for apply_patch-internal data structures
* Limit `PathUri` -> `AbsolutePathBuf` conversion to cases where the
inferred path convention matches the host OS, allows requiring valid
paths to pass to perms check
* Adds `PathConvention::path_segments()` for iterating over path
segments regardless of OS
* Handle cross-platform relative paths in path filename parsing for
sniffing a shell
* Ensure we can apply patches in the wine e2e test
This commit is contained in:
Adam Perry @ OpenAI
2026-06-18 19:31:19 +00:00
committed by GitHub
parent a52a3b5197
commit 0f89dd768c
20 changed files with 626 additions and 370 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/apply-patch/src/invocation.rs
+125 -81
View File
@@ -1,9 +1,7 @@
use std::collections::HashMap;
use std::path::Path;
use std::sync::LazyLock;
use codex_exec_server::ExecutorFileSystem;
use codex_utils_absolute_path::AbsolutePathBuf;
use tree_sitter::Parser;
use tree_sitter::Query;
use tree_sitter::QueryCursor;
@@ -21,6 +19,7 @@ use crate::parser::Hunk;
use crate::parser::ParseError;
use crate::parser::parse_patch;
use crate::unified_diff_from_chunks;
use codex_utils_path_uri::PathConvention;
use codex_utils_path_uri::PathUri;
use std::str::Utf8Error;
use tree_sitter::LanguageError;
@@ -51,15 +50,17 @@ pub enum ExtractHeredocError {
FailedToFindHeredocBody,
}
fn classify_shell_name(shell: &str) -> Option<String> {
std::path::Path::new(shell)
.file_stem()
.and_then(|name| name.to_str())
.map(str::to_ascii_lowercase)
fn classify_shell_name(shell: &str, convention: PathConvention) -> Option<String> {
let basename = convention.path_segments(shell).next_back()?;
let stem = basename
.rsplit_once('.')
.and_then(|(stem, _extension)| (!stem.is_empty()).then_some(stem))
.unwrap_or(basename);
Some(stem.to_ascii_lowercase())
}
fn classify_shell(shell: &str, flag: &str) -> Option<ApplyPatchShell> {
classify_shell_name(shell).and_then(|name| match name.as_str() {
fn classify_shell(shell: &str, flag: &str, convention: PathConvention) -> Option<ApplyPatchShell> {
classify_shell_name(shell, convention).and_then(|name| match name.as_str() {
"bash" | "zsh" | "sh" if matches!(flag, "-lc" | "-c") => Some(ApplyPatchShell::Unix),
"pwsh" | "powershell" if flag.eq_ignore_ascii_case("-command") => {
Some(ApplyPatchShell::PowerShell)
@@ -69,20 +70,24 @@ fn classify_shell(shell: &str, flag: &str) -> Option<ApplyPatchShell> {
})
}
fn can_skip_flag(shell: &str, flag: &str) -> bool {
classify_shell_name(shell).is_some_and(|name| {
fn can_skip_flag(shell: &str, flag: &str, convention: PathConvention) -> bool {
classify_shell_name(shell, convention).is_some_and(|name| {
matches!(name.as_str(), "pwsh" | "powershell") && flag.eq_ignore_ascii_case("-noprofile")
})
}
fn parse_shell_script(argv: &[String]) -> Option<(ApplyPatchShell, &str)> {
fn parse_shell_script<'a>(argv: &'a [String], cwd: &PathUri) -> Option<(ApplyPatchShell, &'a str)> {
let convention = cwd.infer_path_convention()?;
match argv {
[shell, flag, script] => classify_shell(shell, flag).map(|shell_type| {
[shell, flag, script] => classify_shell(shell, flag, convention).map(|shell_type| {
let script = script.as_str();
(shell_type, script)
}),
[shell, skip_flag, flag, script] if can_skip_flag(shell, skip_flag) => {
classify_shell(shell, flag).map(|shell_type| {
[shell, skip_flag, flag, script] => {
if !can_skip_flag(shell, skip_flag, convention) {
return None;
}
classify_shell(shell, flag, convention).map(|shell_type| {
let script = script.as_str();
(shell_type, script)
})
@@ -103,7 +108,8 @@ fn extract_apply_patch_from_shell(
}
// TODO: make private once we remove tests in lib.rs
pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch {
/// `cwd` supplies the path convention used to interpret the shell executable in `argv`.
pub fn maybe_parse_apply_patch(argv: &[String], cwd: &PathUri) -> MaybeApplyPatch {
match argv {
// Direct invocation: apply_patch <patch>
[cmd, body] if APPLY_PATCH_COMMANDS.contains(&cmd.as_str()) => match parse_patch(body) {
@@ -111,7 +117,7 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch {
Err(e) => MaybeApplyPatch::PatchParseError(e),
},
// Shell heredoc form: (optional `cd <path> &&`) apply_patch <<'EOF' ...
_ => match parse_shell_script(argv) {
_ => match parse_shell_script(argv, cwd) {
Some((shell, script)) => match extract_apply_patch_from_shell(shell, script) {
Ok((body, workdir)) => match parse_patch(&body) {
Ok(mut source) => {
@@ -130,11 +136,11 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch {
}
}
/// cwd must be an absolute path so that we can resolve relative paths in the
/// patch.
/// `cwd` must identify an absolute environment-native path so relative patch paths can be
/// resolved without projecting them onto the app-server or exec-server host.
pub async fn maybe_parse_apply_patch_verified(
argv: &[String],
cwd: &AbsolutePathBuf,
cwd: &PathUri,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&codex_exec_server::FileSystemSandboxContext>,
) -> MaybeApplyPatchVerified {
@@ -145,13 +151,13 @@ pub async fn maybe_parse_apply_patch_verified(
{
return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation);
}
if let Some((_, script)) = parse_shell_script(argv)
if let Some((_, script)) = parse_shell_script(argv, cwd)
&& parse_patch(script).is_ok()
{
return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation);
}
match maybe_parse_apply_patch(argv) {
match maybe_parse_apply_patch(argv, cwd) {
MaybeApplyPatch::Body(args) => verify_apply_patch_args(args, cwd, fs, sandbox).await,
MaybeApplyPatch::ShellParseError(e) => MaybeApplyPatchVerified::ShellParseError(e),
MaybeApplyPatch::PatchParseError(e) => MaybeApplyPatchVerified::CorrectnessError(e.into()),
@@ -161,10 +167,22 @@ pub async fn maybe_parse_apply_patch_verified(
pub async fn verify_apply_patch_args(
args: ApplyPatchArgs,
cwd: &AbsolutePathBuf,
cwd: &PathUri,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&codex_exec_server::FileSystemSandboxContext>,
) -> MaybeApplyPatchVerified {
match try_verify_apply_patch_args(args, cwd, fs, sandbox).await {
Ok(action) => MaybeApplyPatchVerified::Body(action),
Err(err) => MaybeApplyPatchVerified::CorrectnessError(err),
}
}
async fn try_verify_apply_patch_args(
args: ApplyPatchArgs,
cwd: &PathUri,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&codex_exec_server::FileSystemSandboxContext>,
) -> Result<ApplyPatchAction, ApplyPatchError> {
let ApplyPatchArgs {
patch,
hunks,
@@ -173,35 +191,24 @@ pub async fn verify_apply_patch_args(
} = args;
let effective_cwd = workdir
.as_ref()
.map(|dir| cwd.join(Path::new(dir)))
.map(|dir| cwd.join(dir))
.transpose()?
.unwrap_or_else(|| cwd.clone());
let mut changes = HashMap::new();
for hunk in hunks {
let path = hunk.resolve_path(&effective_cwd);
let path = hunk.resolve_path(&effective_cwd)?;
match hunk {
Hunk::AddFile { contents, .. } => {
changes.insert(
path.into_path_buf(),
ApplyPatchFileChange::Add { content: contents },
);
changes.insert(path, ApplyPatchFileChange::Add { content: contents });
}
Hunk::DeleteFile { .. } => {
let path_uri = PathUri::from_abs_path(&path);
let content = match fs.read_file_text(&path_uri, sandbox).await {
Ok(content) => content,
Err(e) => {
return MaybeApplyPatchVerified::CorrectnessError(
ApplyPatchError::IoError(IoError {
context: format!("Failed to read {}", path.display()),
source: e,
}),
);
}
};
changes.insert(
path.into_path_buf(),
ApplyPatchFileChange::Delete { content },
);
let content = fs.read_file_text(&path, sandbox).await.map_err(|source| {
ApplyPatchError::IoError(IoError {
context: format!("Failed to read {}", path.inferred_native_path_string()),
source,
})
})?;
changes.insert(path, ApplyPatchFileChange::Delete { content });
}
Hunk::UpdateFile {
move_path, chunks, ..
@@ -210,27 +217,24 @@ pub async fn verify_apply_patch_args(
unified_diff,
content: contents,
..
} = match unified_diff_from_chunks(&path, &chunks, fs, sandbox).await {
Ok(diff) => diff,
Err(e) => {
return MaybeApplyPatchVerified::CorrectnessError(e);
}
};
} = unified_diff_from_chunks(&path, &chunks, fs, sandbox).await?;
changes.insert(
path.into_path_buf(),
path,
ApplyPatchFileChange::Update {
unified_diff,
move_path: move_path.map(|p| effective_cwd.join(p).into_path_buf()),
move_path: move_path
.map(|path| effective_cwd.join(&path.to_string_lossy()))
.transpose()?,
new_content: contents,
},
);
}
}
}
MaybeApplyPatchVerified::Body(ApplyPatchAction {
Ok(ApplyPatchAction {
changes,
patch,
cwd: effective_cwd.into(),
cwd: effective_cwd,
})
}
@@ -392,7 +396,6 @@ mod tests {
use crate::unified_diff_from_chunks;
use assert_matches::assert_matches;
use codex_exec_server::LOCAL_FS;
use codex_utils_absolute_path::test_support::PathExt;
use pretty_assertions::assert_eq;
use std::fs;
use std::path::PathBuf;
@@ -448,8 +451,22 @@ mod tests {
}]
}
#[track_caller]
fn assert_match_args(args: Vec<String>, expected_workdir: Option<&str>) {
match maybe_parse_apply_patch(&args) {
assert_match_args_with_cwd(
args,
&PathUri::parse("file:///workspace").expect("valid POSIX test cwd"),
expected_workdir,
);
}
#[track_caller]
fn assert_match_args_with_cwd(
args: Vec<String>,
cwd: &PathUri,
expected_workdir: Option<&str>,
) {
match maybe_parse_apply_patch(&args, cwd) {
MaybeApplyPatch::Body(ApplyPatchArgs { hunks, workdir, .. }) => {
assert_eq!(workdir.as_deref(), expected_workdir);
assert_eq!(hunks, expected_single_add());
@@ -458,6 +475,7 @@ mod tests {
}
}
#[track_caller]
fn assert_match(script: &str, expected_workdir: Option<&str>) {
let args = args_bash(script);
assert_match_args(args, expected_workdir);
@@ -466,7 +484,10 @@ mod tests {
fn assert_not_match(script: &str) {
let args = args_bash(script);
assert_matches!(
maybe_parse_apply_patch(&args),
maybe_parse_apply_patch(
&args,
&PathUri::parse("file:///workspace").expect("valid POSIX test cwd"),
),
MaybeApplyPatch::NotApplyPatch
);
}
@@ -479,7 +500,7 @@ mod tests {
assert_matches!(
maybe_parse_apply_patch_verified(
&args,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
@@ -496,7 +517,7 @@ mod tests {
assert_matches!(
maybe_parse_apply_patch_verified(
&args,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
@@ -516,7 +537,10 @@ mod tests {
"#,
]);
match maybe_parse_apply_patch(&args) {
match maybe_parse_apply_patch(
&args,
&PathUri::parse("file:///workspace").expect("valid POSIX test cwd"),
) {
MaybeApplyPatch::Body(ApplyPatchArgs { hunks, .. }) => {
assert_eq!(
hunks,
@@ -541,7 +565,10 @@ mod tests {
"#,
]);
match maybe_parse_apply_patch(&args) {
match maybe_parse_apply_patch(
&args,
&PathUri::parse("file:///workspace").expect("valid POSIX test cwd"),
) {
MaybeApplyPatch::Body(ApplyPatchArgs { hunks, .. }) => {
assert_eq!(
hunks,
@@ -580,7 +607,10 @@ mod tests {
PATCH"#,
]);
match maybe_parse_apply_patch(&args) {
match maybe_parse_apply_patch(
&args,
&PathUri::parse("file:///workspace").expect("valid POSIX test cwd"),
) {
MaybeApplyPatch::Body(ApplyPatchArgs { hunks, workdir, .. }) => {
assert_eq!(workdir, None);
assert_eq!(
@@ -614,6 +644,21 @@ PATCH"#,
assert_match_args(args_pwsh(&script), /*expected_workdir*/ None);
}
#[tokio::test]
async fn test_apply_patch_interception_uses_cwd_convention_for_windows_pwsh_path() {
let script = heredoc_script("");
assert_match_args_with_cwd(
strs_to_strings(&[
r"C:\Program Files\PowerShell\7\pwsh.exe",
"-NoProfile",
"-Command",
&script,
]),
&PathUri::parse("file:///C:/windows").expect("valid Windows test cwd"),
/*expected_workdir*/ None,
);
}
#[tokio::test]
async fn test_cmd_heredoc_with_cd() {
let script = heredoc_script("cd foo && ");
@@ -707,9 +752,9 @@ PATCH"#,
_ => panic!("Expected a single UpdateFile hunk"),
};
let path_abs = path.as_path().abs();
let path_uri = PathUri::from_path(&path).expect("absolute test path");
let diff =
unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None)
unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None)
.await
.unwrap();
let expected_diff = r#"@@ -2,2 +2,2 @@
@@ -747,9 +792,9 @@ PATCH"#,
_ => panic!("Expected a single UpdateFile hunk"),
};
let path_abs = path.as_path().abs();
let path_uri = PathUri::from_path(&path).expect("absolute test path");
let diff =
unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None)
unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None)
.await
.unwrap();
let expected_diff = r#"@@ -3 +3,2 @@
@@ -787,7 +832,7 @@ PATCH"#,
let result = maybe_parse_apply_patch_verified(
&argv,
&AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(),
&PathUri::from_path(session_dir.path()).expect("absolute test path"),
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
@@ -799,7 +844,8 @@ PATCH"#,
result,
MaybeApplyPatchVerified::Body(ApplyPatchAction {
changes: HashMap::from([(
session_dir.path().join(relative_path),
PathUri::from_path(session_dir.path().join(relative_path))
.expect("absolute test path"),
ApplyPatchFileChange::Update {
unified_diff: r#"@@ -1 +1 @@
-session directory content
@@ -811,9 +857,7 @@ PATCH"#,
},
)]),
patch: argv[1].clone(),
cwd: AbsolutePathBuf::from_absolute_path(session_dir.path())
.unwrap()
.into(),
cwd: PathUri::from_path(session_dir.path()).expect("absolute test path"),
})
);
}
@@ -843,7 +887,7 @@ PATCH"#,
let result = maybe_parse_apply_patch_verified(
&argv,
&AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(),
&PathUri::from_path(session_dir.path()).expect("absolute test path"),
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
@@ -858,18 +902,18 @@ PATCH"#,
worktree_dir.as_path()
);
let source_path = worktree_dir.join(source_name);
let source_path =
PathUri::from_path(worktree_dir.join(source_name)).expect("absolute test path");
let change = action
.changes()
.get(source_path.as_path())
.get(&source_path)
.expect("source file change present");
match change {
ApplyPatchFileChange::Update { move_path, .. } => {
assert_eq!(
move_path.as_deref(),
Some(worktree_dir.join(dest_name).as_path())
);
let expected_move_path =
PathUri::from_path(worktree_dir.join(dest_name)).expect("absolute test path");
assert_eq!(move_path.as_ref(), Some(&expected_move_path));
}
other => panic!("expected update change, got {other:?}"),
}
@@ -879,7 +923,7 @@ PATCH"#,
async fn test_unreadable_destinations_still_verify() {
let session_dir = tempdir().unwrap();
fs::write(session_dir.path().join("binary.dat"), [0xff, 0xfe, 0xfd]).unwrap();
let cwd = AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap();
let cwd = PathUri::from_path(session_dir.path()).expect("absolute test path");
let add_argv = vec![
"apply_patch".to_string(),
"*** Begin Patch\n*** Add File: binary.dat\n+text\n*** End Patch".to_string(),
@@ -922,7 +966,7 @@ PATCH"#,
let result = maybe_parse_apply_patch_verified(
&argv,
&AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(),
&PathUri::from_path(session_dir.path()).expect("absolute test path"),
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
codex-rs/apply-patch/src/lib.rs
+137 -116
View File
@@ -6,7 +6,6 @@ mod streaming_parser;
use std::collections::HashMap;
use std::io;
use std::path::Path;
use std::path::PathBuf;
use anyhow::Context;
@@ -15,8 +14,8 @@ use codex_exec_server::CreateDirectoryOptions;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::FileSystemSandboxContext;
use codex_exec_server::RemoveOptions;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use codex_utils_path_uri::PathUriParseError;
pub use parser::Hunk;
pub use parser::ParseError;
use parser::ParseError::*;
@@ -50,6 +49,9 @@ pub enum ApplyPatchError {
/// Error that occurs while computing replacements when applying patch chunks
#[error("{0}")]
ComputeReplacements(String),
/// A patch path could not be resolved as a path URI.
#[error(transparent)]
PathUri(#[from] PathUriParseError),
/// A raw patch body was provided without an explicit `apply_patch` invocation.
#[error(
"patch detected without explicit call to apply_patch. Rerun as [\"apply_patch\", \"<patch>\"]"
@@ -109,7 +111,7 @@ pub enum ApplyPatchFileChange {
},
Update {
unified_diff: String,
move_path: Option<PathBuf>,
move_path: Option<PathUri>,
/// new_content that will result after the unified_diff is applied.
new_content: String,
},
@@ -134,7 +136,7 @@ pub enum MaybeApplyPatchVerified {
/// construction, all paths should be absolute paths.
#[derive(Debug, PartialEq)]
pub struct ApplyPatchAction {
changes: HashMap<PathBuf, ApplyPatchFileChange>,
changes: HashMap<PathUri, ApplyPatchFileChange>,
/// The raw patch argument that can be used to apply the patch. i.e., if the
/// original arg was parsed in "lenient" mode with a
@@ -151,18 +153,15 @@ impl ApplyPatchAction {
}
/// Returns the changes that would be made by applying the patch.
pub fn changes(&self) -> &HashMap<PathBuf, ApplyPatchFileChange> {
pub fn changes(&self) -> &HashMap<PathUri, ApplyPatchFileChange> {
&self.changes
}
/// Should be used exclusively for testing. (Not worth the overhead of
/// creating a feature flag for this.)
pub fn new_add_for_test(path: &AbsolutePathBuf, content: String) -> Self {
pub fn new_add_for_test(path: &PathUri, content: String) -> Self {
#[expect(clippy::expect_used)]
let filename = path
.file_name()
.expect("path should not be empty")
.to_string_lossy();
let filename = path.basename().expect("path should not be empty");
let patch = format!(
r#"*** Begin Patch
*** Update File: {filename}
@@ -170,11 +169,11 @@ impl ApplyPatchAction {
+ {content}
*** End Patch"#,
);
let changes = HashMap::from([(path.to_path_buf(), ApplyPatchFileChange::Add { content })]);
let changes = HashMap::from([(path.clone(), ApplyPatchFileChange::Add { content })]);
#[expect(clippy::expect_used)]
Self {
changes,
cwd: path.parent().expect("path should have parent").into(),
cwd: path.parent().expect("path should have parent"),
patch,
}
}
@@ -276,7 +275,7 @@ impl ApplyPatchFailure {
/// Applies the patch and prints the result to stdout/stderr.
pub async fn apply_patch(
patch: &str,
cwd: &AbsolutePathBuf,
cwd: &PathUri,
stdout: &mut impl std::io::Write,
stderr: &mut impl std::io::Write,
fs: &dyn ExecutorFileSystem,
@@ -315,7 +314,7 @@ pub async fn apply_patch(
/// Applies hunks and continues to update stdout/stderr
pub async fn apply_hunks(
hunks: &[Hunk],
cwd: &AbsolutePathBuf,
cwd: &PathUri,
stdout: &mut impl std::io::Write,
stderr: &mut impl std::io::Write,
fs: &dyn ExecutorFileSystem,
@@ -361,7 +360,7 @@ pub struct AffectedPaths {
/// Returns an error if the patch could not be applied.
async fn apply_hunks_to_files(
hunks: &[Hunk],
cwd: &AbsolutePathBuf,
cwd: &PathUri,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
delta: &mut AppliedPatchDelta,
@@ -388,26 +387,26 @@ async fn apply_hunks_to_files(
};
}
// TODO(anp): Carry PathUri through committed patch deltas and the turn diff tracker.
for hunk in hunks {
let affected_path = hunk.path().to_path_buf();
let path_abs = hunk.resolve_path(cwd);
let path_uri = PathUri::from_abs_path(&path_abs);
let path_uri = hunk.resolve_path(cwd)?;
match hunk {
Hunk::AddFile { contents, .. } => {
let overwritten_content =
read_optional_file_text_for_delta(&path_abs, fs, sandbox, &mut delta.exact)
read_optional_file_text_for_delta(&path_uri, fs, sandbox, &mut delta.exact)
.await;
try_write!(
write_file_with_missing_parent_retry(
fs,
&path_abs,
&path_uri,
contents.clone().into_bytes(),
sandbox,
)
.await
);
delta.changes.push(AppliedPatchChange {
path: path_abs.into_path_buf(),
path: path_uri.to_path_buf(),
change: AppliedPatchFileChange::Add {
content: contents.clone(),
overwritten_content,
@@ -416,14 +415,19 @@ async fn apply_hunks_to_files(
added.push(affected_path);
}
Hunk::DeleteFile { .. } => {
note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta.exact).await;
note_existing_path_delta_support(&path_uri, fs, sandbox, &mut delta.exact).await;
let deleted_content = fs.read_file_text(&path_uri, sandbox).await.ok();
if deleted_content.is_none() {
delta.exact = false;
}
ensure_not_directory(&path_abs, fs, sandbox)
ensure_not_directory(&path_uri, fs, sandbox)
.await
.with_context(|| format!("Failed to delete file {}", path_abs.display()))?;
.with_context(|| {
format!(
"Failed to delete file {}",
path_uri.inferred_native_path_string()
)
})?;
if let Err(error) = fs
.remove(
&path_uri,
@@ -434,10 +438,15 @@ async fn apply_hunks_to_files(
sandbox,
)
.await
.with_context(|| format!("Failed to delete file {}", path_abs.display()))
.with_context(|| {
format!(
"Failed to delete file {}",
path_uri.inferred_native_path_string()
)
})
{
delta.exact &= remove_failure_was_side_effect_free(
&path_abs,
&path_uri,
deleted_content.as_deref(),
fs,
sandbox,
@@ -447,7 +456,7 @@ async fn apply_hunks_to_files(
}
if let Some(content) = deleted_content {
delta.changes.push(AppliedPatchChange {
path: path_abs.into_path_buf(),
path: path_uri.to_path_buf(),
change: AppliedPatchFileChange::Delete { content },
});
}
@@ -456,20 +465,20 @@ async fn apply_hunks_to_files(
Hunk::UpdateFile {
move_path, chunks, ..
} => {
note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta.exact).await;
note_existing_path_delta_support(&path_uri, fs, sandbox, &mut delta.exact).await;
let AppliedPatch {
original_contents,
new_contents,
} = derive_new_contents_from_chunks(&path_abs, chunks, fs, sandbox).await?;
} = derive_new_contents_from_chunks(&path_uri, chunks, fs, sandbox).await?;
if let Some(dest) = move_path {
let dest_abs = AbsolutePathBuf::resolve_path_against_base(dest, cwd);
let dest_uri = cwd.join(&dest.to_string_lossy())?;
let overwritten_move_content =
read_optional_file_text_for_delta(&dest_abs, fs, sandbox, &mut delta.exact)
read_optional_file_text_for_delta(&dest_uri, fs, sandbox, &mut delta.exact)
.await;
try_write!(
write_file_with_missing_parent_retry(
fs,
&dest_abs,
&dest_uri,
new_contents.clone().into_bytes(),
sandbox,
)
@@ -477,16 +486,19 @@ async fn apply_hunks_to_files(
);
let dest_write_change_index = delta.changes.len();
delta.changes.push(AppliedPatchChange {
path: dest_abs.to_path_buf(),
path: dest_uri.to_path_buf(),
change: AppliedPatchFileChange::Add {
content: new_contents.clone(),
overwritten_content: overwritten_move_content.clone(),
},
});
ensure_not_directory(&path_abs, fs, sandbox)
ensure_not_directory(&path_uri, fs, sandbox)
.await
.with_context(|| {
format!("Failed to remove original {}", path_abs.display())
format!(
"Failed to remove original {}",
path_uri.inferred_native_path_string()
)
})?;
if let Err(error) = fs
.remove(
@@ -499,11 +511,14 @@ async fn apply_hunks_to_files(
)
.await
.with_context(|| {
format!("Failed to remove original {}", path_abs.display())
format!(
"Failed to remove original {}",
path_uri.inferred_native_path_string()
)
})
{
delta.exact &= remove_failure_was_side_effect_free(
&path_abs,
&path_uri,
Some(&original_contents),
fs,
sandbox,
@@ -512,9 +527,9 @@ async fn apply_hunks_to_files(
return Err(error);
}
delta.changes[dest_write_change_index] = AppliedPatchChange {
path: path_abs.into_path_buf(),
path: path_uri.to_path_buf(),
change: AppliedPatchFileChange::Update {
move_path: Some(dest_abs.into_path_buf()),
move_path: Some(dest_uri.to_path_buf()),
old_content: original_contents,
overwritten_move_content,
new_content: new_contents,
@@ -527,11 +542,11 @@ async fn apply_hunks_to_files(
.await
.with_context(|| format!(
"Failed to write file {}",
path_abs.display()
path_uri.inferred_native_path_string()
))
);
delta.changes.push(AppliedPatchChange {
path: path_abs.into_path_buf(),
path: path_uri.to_path_buf(),
change: AppliedPatchFileChange::Update {
move_path: None,
old_content: original_contents,
@@ -552,12 +567,11 @@ async fn apply_hunks_to_files(
}
async fn ensure_not_directory(
path: &AbsolutePathBuf,
path: &PathUri,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
) -> io::Result<()> {
let path_uri = PathUri::from_abs_path(path);
let metadata = fs.get_metadata(&path_uri, sandbox).await?;
let metadata = fs.get_metadata(path, sandbox).await?;
if metadata.is_directory {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
@@ -568,15 +582,14 @@ async fn ensure_not_directory(
}
async fn remove_failure_was_side_effect_free(
path: &AbsolutePathBuf,
path: &PathUri,
expected_content: Option<&str>,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
) -> bool {
let path_uri = PathUri::from_abs_path(path);
match expected_content {
Some(expected_content) => fs
.read_file_text(&path_uri, sandbox)
.read_file_text(path, sandbox)
.await
.is_ok_and(|content| content == expected_content),
None => false,
@@ -584,14 +597,13 @@ async fn remove_failure_was_side_effect_free(
}
async fn read_optional_file_text_for_delta(
path: &AbsolutePathBuf,
path: &PathUri,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
exact: &mut bool,
) -> Option<String> {
note_existing_path_delta_support(path, fs, sandbox, exact).await;
let path_uri = PathUri::from_abs_path(path);
match fs.read_file_text(&path_uri, sandbox).await {
match fs.read_file_text(path, sandbox).await {
Ok(content) => Some(content),
Err(source) if source.kind() == io::ErrorKind::NotFound => None,
Err(_) => {
@@ -602,13 +614,12 @@ async fn read_optional_file_text_for_delta(
}
async fn note_existing_path_delta_support(
path: &AbsolutePathBuf,
path: &PathUri,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
exact: &mut bool,
) {
let path_uri = PathUri::from_abs_path(path);
match fs.get_metadata(&path_uri, sandbox).await {
match fs.get_metadata(path, sandbox).await {
Ok(metadata) if metadata.is_file && !metadata.is_symlink => {}
Ok(_) => *exact = false,
Err(source) if source.kind() == io::ErrorKind::NotFound => {}
@@ -618,37 +629,39 @@ async fn note_existing_path_delta_support(
async fn write_file_with_missing_parent_retry(
fs: &dyn ExecutorFileSystem,
path_abs: &AbsolutePathBuf,
path: &PathUri,
contents: Vec<u8>,
sandbox: Option<&FileSystemSandboxContext>,
) -> anyhow::Result<()> {
let path_uri = PathUri::from_abs_path(path_abs);
match fs.write_file(&path_uri, contents.clone(), sandbox).await {
match fs.write_file(path, contents.clone(), sandbox).await {
Ok(()) => Ok(()),
Err(err) if err.kind() == io::ErrorKind::NotFound => {
if let Some(parent_abs) = path_abs.parent() {
let parent_uri = PathUri::from_abs_path(&parent_abs);
fs.create_directory(
&parent_uri,
CreateDirectoryOptions { recursive: true },
sandbox,
)
if let Some(parent) = path.parent() {
fs.create_directory(&parent, CreateDirectoryOptions { recursive: true }, sandbox)
.await
.with_context(|| {
format!(
"Failed to create parent directories for {}",
path.inferred_native_path_string()
)
})?;
}
fs.write_file(path, contents, sandbox)
.await
.with_context(|| {
format!(
"Failed to create parent directories for {}",
path_abs.display()
"Failed to write file {}",
path.inferred_native_path_string()
)
})?;
}
fs.write_file(&path_uri, contents, sandbox)
.await
.with_context(|| format!("Failed to write file {}", path_abs.display()))?;
Ok(())
}
Err(err) => {
Err(err).with_context(|| format!("Failed to write file {}", path_abs.display()))
}
Err(err) => Err(err).with_context(|| {
format!(
"Failed to write file {}",
path.inferred_native_path_string()
)
}),
}
}
@@ -660,15 +673,17 @@ struct AppliedPatch {
/// Return *only* the new file contents (joined into a single `String`) after
/// applying the chunks to the file at `path`.
async fn derive_new_contents_from_chunks(
path_abs: &AbsolutePathBuf,
path: &PathUri,
chunks: &[UpdateFileChunk],
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
) -> std::result::Result<AppliedPatch, ApplyPatchError> {
let path_uri = PathUri::from_abs_path(path_abs);
let original_contents = fs.read_file_text(&path_uri, sandbox).await.map_err(|err| {
let original_contents = fs.read_file_text(path, sandbox).await.map_err(|err| {
ApplyPatchError::IoError(IoError {
context: format!("Failed to read file to update {}", path_abs.display()),
context: format!(
"Failed to read file to update {}",
path.inferred_native_path_string()
),
source: err,
})
})?;
@@ -681,7 +696,8 @@ async fn derive_new_contents_from_chunks(
original_lines.pop();
}
let replacements = compute_replacements(&original_lines, path_abs.as_path(), chunks)?;
let path_text = path.inferred_native_path_string();
let replacements = compute_replacements(&original_lines, &path_text, chunks)?;
let new_lines = apply_replacements(original_lines, &replacements);
let mut new_lines = new_lines;
if !new_lines.last().is_some_and(String::is_empty) {
@@ -699,7 +715,7 @@ async fn derive_new_contents_from_chunks(
/// `(start_index, old_len, new_lines)`.
fn compute_replacements(
original_lines: &[String],
path: &Path,
path: &str,
chunks: &[UpdateFileChunk],
) -> std::result::Result<Vec<(usize, usize, Vec<String>)>, ApplyPatchError> {
let mut replacements: Vec<(usize, usize, Vec<String>)> = Vec::new();
@@ -718,9 +734,7 @@ fn compute_replacements(
line_index = idx + 1;
} else {
return Err(ApplyPatchError::ComputeReplacements(format!(
"Failed to find context '{}' in {}",
ctx_line,
path.display()
"Failed to find context '{ctx_line}' in {path}"
)));
}
}
@@ -776,7 +790,7 @@ fn compute_replacements(
} else {
return Err(ApplyPatchError::ComputeReplacements(format!(
"Failed to find expected lines in {}:\n{}",
path.display(),
path,
chunk.old_lines.join("\n"),
)));
}
@@ -824,16 +838,16 @@ pub struct ApplyPatchFileUpdate {
}
pub async fn unified_diff_from_chunks(
path_abs: &AbsolutePathBuf,
path: &PathUri,
chunks: &[UpdateFileChunk],
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
) -> std::result::Result<ApplyPatchFileUpdate, ApplyPatchError> {
unified_diff_from_chunks_with_context(path_abs, chunks, /*context*/ 1, fs, sandbox).await
unified_diff_from_chunks_with_context(path, chunks, /*context*/ 1, fs, sandbox).await
}
pub async fn unified_diff_from_chunks_with_context(
path_abs: &AbsolutePathBuf,
path: &PathUri,
chunks: &[UpdateFileChunk],
context: usize,
fs: &dyn ExecutorFileSystem,
@@ -842,7 +856,7 @@ pub async fn unified_diff_from_chunks_with_context(
let AppliedPatch {
original_contents,
new_contents,
} = derive_new_contents_from_chunks(path_abs, chunks, fs, sandbox).await?;
} = derive_new_contents_from_chunks(path, chunks, fs, sandbox).await?;
let text_diff = TextDiff::from_lines(&original_contents, &new_contents);
let unified_diff = text_diff.unified_diff().context_radius(context).to_string();
Ok(ApplyPatchFileUpdate {
@@ -875,7 +889,6 @@ pub fn print_summary(
mod tests {
use super::*;
use codex_exec_server::LOCAL_FS;
use codex_utils_absolute_path::test_support::PathExt;
use pretty_assertions::assert_eq;
use std::fs;
use std::string::ToString;
@@ -900,7 +913,7 @@ mod tests {
let mut stderr = Vec::new();
apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -924,7 +937,7 @@ mod tests {
#[tokio::test]
async fn test_apply_patch_hunks_accept_relative_and_absolute_paths() {
let dir = tempdir().unwrap();
let cwd = dir.path().abs();
let cwd = PathUri::from_path(dir.path()).expect("absolute test path");
let relative_add = dir.path().join("relative-add.txt");
let absolute_add = dir.path().join("absolute-add.txt");
let relative_delete = dir.path().join("relative-delete.txt");
@@ -1003,7 +1016,7 @@ mod tests {
let mut stderr = Vec::new();
apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -1039,7 +1052,7 @@ mod tests {
let mut stderr = Vec::new();
apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -1079,7 +1092,7 @@ mod tests {
let mut stderr = Vec::new();
apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -1123,7 +1136,7 @@ mod tests {
let mut stderr = Vec::new();
let failure = apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -1183,7 +1196,7 @@ mod tests {
let mut stderr = Vec::new();
apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -1241,7 +1254,7 @@ mod tests {
let mut stderr = Vec::new();
apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -1285,7 +1298,7 @@ mod tests {
let mut stderr = Vec::new();
apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -1328,7 +1341,7 @@ mod tests {
let mut stderr = Vec::new();
apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -1378,9 +1391,9 @@ mod tests {
[Hunk::UpdateFile { chunks, .. }] => chunks,
_ => panic!("Expected a single UpdateFile hunk"),
};
let path_abs = path.as_path().abs();
let path_uri = PathUri::from_path(&path).expect("absolute test path");
let diff = unified_diff_from_chunks(
&path_abs,
&path_uri,
update_file_chunks,
LOCAL_FS.as_ref(),
/*sandbox*/ None,
@@ -1426,11 +1439,15 @@ mod tests {
_ => panic!("Expected a single UpdateFile hunk"),
};
let path_abs = path.as_path().abs();
let diff =
unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None)
.await
.unwrap();
let resolved_path = PathUri::from_path(&path).expect("absolute test path");
let diff = unified_diff_from_chunks(
&resolved_path,
chunks,
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
.await
.unwrap();
let expected_diff = r#"@@ -1,2 +1,2 @@
-foo
+FOO
@@ -1468,11 +1485,15 @@ mod tests {
_ => panic!("Expected a single UpdateFile hunk"),
};
let path_abs = path.as_path().abs();
let diff =
unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None)
.await
.unwrap();
let resolved_path = PathUri::from_path(&path).expect("absolute test path");
let diff = unified_diff_from_chunks(
&resolved_path,
chunks,
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
.await
.unwrap();
let expected_diff = r#"@@ -2,2 +2,2 @@
bar
-baz
@@ -1508,9 +1529,9 @@ mod tests {
_ => panic!("Expected a single UpdateFile hunk"),
};
let path_abs = path.as_path().abs();
let path_uri = PathUri::from_path(&path).expect("absolute test path");
let diff =
unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None)
unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None)
.await
.unwrap();
let expected_diff = r#"@@ -3 +3,2 @@
@@ -1559,9 +1580,9 @@ mod tests {
_ => panic!("Expected a single UpdateFile hunk"),
};
let path_abs = path.as_path().abs();
let path_uri = PathUri::from_path(&path).expect("absolute test path");
let diff =
unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None)
unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None)
.await
.unwrap();
@@ -1589,7 +1610,7 @@ mod tests {
let mut stderr = Vec::new();
apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -1627,7 +1648,7 @@ g
let mut stderr = Vec::new();
let result = apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
@@ -1646,7 +1667,7 @@ g
let dir = tempdir().unwrap();
let path = dir.path().join("binary.dat");
fs::write(dir.path().join("source.txt"), "before\n").unwrap();
let cwd = AbsolutePathBuf::from_absolute_path(dir.path()).unwrap();
let cwd = PathUri::from_path(dir.path()).expect("absolute test path");
for patch in [
wrap_patch("*** Add File: binary.dat\n+text"),
@@ -1684,7 +1705,7 @@ g
let mut stderr = Vec::new();
let delta = apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&PathUri::from_path(dir.path()).expect("absolute test path"),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
codex-rs/apply-patch/src/parser.rs
+12 -11
View File
@@ -25,9 +25,10 @@
//! leading/trailing whitespace around patch markers.
use crate::ApplyPatchArgs;
use crate::streaming_parser::StreamingPatchParser;
use codex_utils_absolute_path::AbsolutePathBuf;
#[cfg(test)]
use codex_utils_absolute_path::test_support::PathBufExt;
use codex_utils_path_uri::PathUri;
use codex_utils_path_uri::PathUriParseError;
use std::path::Path;
use std::path::PathBuf;
@@ -81,12 +82,12 @@ pub enum Hunk {
}
impl Hunk {
pub fn resolve_path(&self, cwd: &AbsolutePathBuf) -> AbsolutePathBuf {
pub fn resolve_path(&self, cwd: &PathUri) -> Result<PathUri, PathUriParseError> {
let path = match self {
Hunk::UpdateFile { path, .. } => path,
Hunk::AddFile { .. } | Hunk::DeleteFile { .. } => self.path(),
};
AbsolutePathBuf::resolve_path_against_base(path, cwd)
cwd.join(&path.to_string_lossy())
}
/// Returns the path affected by this hunk, using the move destination for rename hunks.
@@ -479,7 +480,7 @@ fn test_parse_patch_accepts_relative_and_absolute_hunk_paths() {
#[test]
fn test_hunk_resolve_path_accepts_relative_and_absolute_paths() {
let cwd_dir = tempfile::tempdir().unwrap();
let cwd = cwd_dir.path().to_path_buf().abs();
let cwd = PathUri::from_path(cwd_dir.path()).unwrap();
let absolute_dir = tempfile::tempdir().unwrap();
let absolute_add = absolute_dir.path().join("absolute-add.py").abs();
let absolute_delete = absolute_dir.path().join("absolute-delete.py").abs();
@@ -491,13 +492,13 @@ fn test_hunk_resolve_path_accepts_relative_and_absolute_paths() {
path: PathBuf::from("relative-add.py"),
contents: String::new(),
},
cwd.join("relative-add.py"),
cwd.join("relative-add.py").unwrap(),
),
(
DeleteFile {
path: PathBuf::from("relative-delete.py"),
},
cwd.join("relative-delete.py"),
cwd.join("relative-delete.py").unwrap(),
),
(
UpdateFile {
@@ -505,20 +506,20 @@ fn test_hunk_resolve_path_accepts_relative_and_absolute_paths() {
move_path: None,
chunks: Vec::new(),
},
cwd.join("relative-update.py"),
cwd.join("relative-update.py").unwrap(),
),
(
AddFile {
path: absolute_add.to_path_buf(),
contents: String::new(),
},
absolute_add,
PathUri::from_abs_path(&absolute_add),
),
(
DeleteFile {
path: absolute_delete.to_path_buf(),
},
absolute_delete,
PathUri::from_abs_path(&absolute_delete),
),
(
UpdateFile {
@@ -526,10 +527,10 @@ fn test_hunk_resolve_path_accepts_relative_and_absolute_paths() {
move_path: None,
chunks: Vec::new(),
},
absolute_update,
PathUri::from_abs_path(&absolute_update),
),
] {
assert_eq!(hunk.resolve_path(&cwd), expected_path);
assert_eq!(hunk.resolve_path(&cwd), Ok(expected_path));
}
}
codex-rs/apply-patch/src/standalone_executable.rs
+2
View File
@@ -65,6 +65,8 @@ pub fn run_main() -> i32 {
return 1;
}
};
// TODO(anp): Discover the standalone executable cwd as PathUri directly.
let cwd = codex_utils_path_uri::PathUri::from_abs_path(&cwd);
match runtime.block_on(crate::apply_patch(
&patch_arg,
&cwd,