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

Make turn diff tracking operation backed (#21180)

Browse Source 
## Summary
- replace filesystem-based turn diff tracking with an operation-backed
accumulator
- preserve enough verified apply_patch state to render move-overwrite
cases correctly
- keep the turn/diff/updated contract intact while removing remote-only
turn-diff test skips

This takes the assumption that no 3P services rely on the output format
of `apply_patch`

## Why
For the CCA file system isolation push

---------

Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
jif-oai
2026-05-07 11:33:47 +02:00
committed by GitHub
Unverified
parent b2268999fe
commit f7e8ff8e50
15 changed files with 1050 additions and 898 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/apply-patch/src/invocation.rs
+61 -1
View File
@@ -193,6 +193,7 @@ pub async fn maybe_parse_apply_patch_verified(
let ApplyPatchFileUpdate {
unified_diff,
content: contents,
..
} = match unified_diff_from_chunks(&path, &chunks, fs, sandbox).await {
Ok(diff) => diff,
Err(e) => {
@@ -707,6 +708,7 @@ PATCH"#,
"#;
let expected = ApplyPatchFileUpdate {
unified_diff: expected_diff.to_string(),
original_content: "foo\nbar\nbaz\n".to_string(),
content: "foo\nbar\nBAZ\n".to_string(),
};
assert_eq!(expected, diff);
@@ -745,6 +747,7 @@ PATCH"#,
"#;
let expected = ApplyPatchFileUpdate {
unified_diff: expected_diff.to_string(),
original_content: "foo\nbar\nbaz\n".to_string(),
content: "foo\nbar\nbaz\nquux\n".to_string(),
};
assert_eq!(expected, diff);
@@ -839,9 +842,10 @@ PATCH"#,
assert_eq!(action.cwd.as_path(), worktree_dir.as_path());
let source_path = worktree_dir.join(source_name);
let change = action
.changes()
.get(&worktree_dir.join(source_name))
.get(source_path.as_path())
.expect("source file change present");
match change {
@@ -854,4 +858,60 @@ PATCH"#,
other => panic!("expected update change, got {other:?}"),
}
}
#[tokio::test]
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 add_argv = vec![
"apply_patch".to_string(),
"*** Begin Patch\n*** Add File: binary.dat\n+text\n*** End Patch".to_string(),
];
fs::write(session_dir.path().join("source.txt"), "before\n").unwrap();
let move_argv = vec![
"apply_patch".to_string(),
"*** Begin Patch\n*** Update File: source.txt\n*** Move to: binary.dat\n@@\n-before\n+after\n*** End Patch".to_string(),
];
for argv in [add_argv, move_argv] {
let result = maybe_parse_apply_patch_verified(
&argv,
&cwd,
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
.await;
assert!(matches!(result, MaybeApplyPatchVerified::Body(_)));
}
}
#[cfg(unix)]
#[tokio::test]
async fn test_delete_symlink_still_verifies() {
use std::os::unix::fs::symlink;
let session_dir = tempdir().unwrap();
fs::write(session_dir.path().join("target.txt"), "target\n").unwrap();
symlink(
session_dir.path().join("target.txt"),
session_dir.path().join("link.txt"),
)
.unwrap();
let argv = vec![
"apply_patch".to_string(),
"*** Begin Patch\n*** Delete File: link.txt\n*** End Patch".to_string(),
];
let result = maybe_parse_apply_patch_verified(
&argv,
&AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(),
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
.await;
assert!(matches!(result, MaybeApplyPatchVerified::Body(_)));
}
}
codex-rs/apply-patch/src/lib.rs
+208 -17
View File
@@ -180,6 +180,51 @@ impl ApplyPatchAction {
}
}
/// Textual file changes that were actually committed while applying a patch.
#[derive(Clone, Debug, PartialEq)]
pub struct AppliedPatchDelta {
changes: Vec<AppliedPatchChange>,
exact: bool,
}
impl AppliedPatchDelta {
fn new(changes: Vec<AppliedPatchChange>, exact: bool) -> Self {
Self { changes, exact }
}
pub fn changes(&self) -> &[AppliedPatchChange] {
&self.changes
}
pub fn is_exact(&self) -> bool {
self.exact
}
}
/// A committed file change, preserved in the order it was applied.
#[derive(Clone, Debug, PartialEq)]
pub struct AppliedPatchChange {
pub path: PathBuf,
pub change: AppliedPatchFileChange,
}
#[derive(Clone, Debug, PartialEq)]
pub enum AppliedPatchFileChange {
Add {
content: String,
overwritten_content: Option<String>,
},
Delete {
content: String,
},
Update {
move_path: Option<PathBuf>,
old_content: String,
overwritten_move_content: Option<String>,
new_content: String,
},
}
/// Applies the patch and prints the result to stdout/stderr.
pub async fn apply_patch(
patch: &str,
@@ -188,7 +233,7 @@ pub async fn apply_patch(
stderr: &mut impl std::io::Write,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
) -> Result<(), ApplyPatchError> {
) -> Result<AppliedPatchDelta, ApplyPatchError> {
let hunks = match parse_patch(patch) {
Ok(source) => source.hunks,
Err(e) => {
@@ -211,9 +256,7 @@ pub async fn apply_patch(
}
};
apply_hunks(&hunks, cwd, stdout, stderr, fs, sandbox).await?;
Ok(())
apply_hunks(&hunks, cwd, stdout, stderr, fs, sandbox).await
}
/// Applies hunks and continues to update stdout/stderr
@@ -224,12 +267,12 @@ pub async fn apply_hunks(
stderr: &mut impl std::io::Write,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
) -> Result<(), ApplyPatchError> {
) -> Result<AppliedPatchDelta, ApplyPatchError> {
// Delegate to a helper that applies each hunk to the filesystem.
match apply_hunks_to_files(hunks, cwd, fs, sandbox).await {
Ok(affected) => {
print_summary(&affected, stdout).map_err(ApplyPatchError::from)?;
Ok(())
Ok(applied) => {
print_summary(&applied.affected_paths, stdout).map_err(ApplyPatchError::from)?;
Ok(applied.delta)
}
Err(err) => {
let msg = err.to_string();
@@ -256,6 +299,11 @@ pub struct AffectedPaths {
pub deleted: Vec<PathBuf>,
}
struct AppliedHunks {
affected_paths: AffectedPaths,
delta: AppliedPatchDelta,
}
/// Apply the hunks to the filesystem, returning which files were added, modified, or deleted.
/// Returns an error if the patch could not be applied.
async fn apply_hunks_to_files(
@@ -263,7 +311,7 @@ async fn apply_hunks_to_files(
cwd: &AbsolutePathBuf,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
) -> anyhow::Result<AffectedPaths> {
) -> anyhow::Result<AppliedHunks> {
if hunks.is_empty() {
anyhow::bail!("No files were modified.");
}
@@ -271,11 +319,16 @@ async fn apply_hunks_to_files(
let mut added: Vec<PathBuf> = Vec::new();
let mut modified: Vec<PathBuf> = Vec::new();
let mut deleted: Vec<PathBuf> = Vec::new();
let mut delta_changes = Vec::new();
let mut delta_exact = true;
for hunk in hunks {
let affected_path = hunk.path().to_path_buf();
let path_abs = 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)
.await;
write_file_with_missing_parent_retry(
fs,
&path_abs,
@@ -283,9 +336,21 @@ async fn apply_hunks_to_files(
sandbox,
)
.await?;
delta_changes.push(AppliedPatchChange {
path: path_abs.into_path_buf(),
change: AppliedPatchFileChange::Add {
content: contents.clone(),
overwritten_content,
},
});
added.push(affected_path);
}
Hunk::DeleteFile { .. } => {
note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta_exact).await;
let deleted_content = fs.read_file_text(&path_abs, sandbox).await.ok();
if deleted_content.is_none() {
delta_exact = false;
}
let result: io::Result<()> = async {
let metadata = fs.get_metadata(&path_abs, sandbox).await?;
if metadata.is_directory {
@@ -306,19 +371,31 @@ async fn apply_hunks_to_files(
}
.await;
result.with_context(|| format!("Failed to delete file {}", path_abs.display()))?;
if let Some(content) = deleted_content {
delta_changes.push(AppliedPatchChange {
path: path_abs.into_path_buf(),
change: AppliedPatchFileChange::Delete { content },
});
}
deleted.push(affected_path);
}
Hunk::UpdateFile {
move_path, chunks, ..
} => {
let AppliedPatch { new_contents, .. } =
derive_new_contents_from_chunks(&path_abs, chunks, fs, sandbox).await?;
note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta_exact).await;
let AppliedPatch {
original_contents,
new_contents,
} = derive_new_contents_from_chunks(&path_abs, chunks, fs, sandbox).await?;
if let Some(dest) = move_path {
let dest_abs = AbsolutePathBuf::resolve_path_against_base(dest, cwd);
let overwritten_move_content =
read_optional_file_text_for_delta(&dest_abs, fs, sandbox, &mut delta_exact)
.await;
write_file_with_missing_parent_retry(
fs,
&dest_abs,
new_contents.into_bytes(),
new_contents.clone().into_bytes(),
sandbox,
)
.await?;
@@ -344,23 +421,75 @@ async fn apply_hunks_to_files(
result.with_context(|| {
format!("Failed to remove original {}", path_abs.display())
})?;
delta_changes.push(AppliedPatchChange {
path: path_abs.into_path_buf(),
change: AppliedPatchFileChange::Update {
move_path: Some(dest_abs.into_path_buf()),
old_content: original_contents,
overwritten_move_content,
new_content: new_contents,
},
});
modified.push(affected_path);
} else {
fs.write_file(&path_abs, new_contents.into_bytes(), sandbox)
fs.write_file(&path_abs, new_contents.clone().into_bytes(), sandbox)
.await
.with_context(|| format!("Failed to write file {}", path_abs.display()))?;
delta_changes.push(AppliedPatchChange {
path: path_abs.into_path_buf(),
change: AppliedPatchFileChange::Update {
move_path: None,
old_content: original_contents,
overwritten_move_content: None,
new_content: new_contents,
},
});
modified.push(affected_path);
}
}
}
}
Ok(AffectedPaths {
added,
modified,
deleted,
Ok(AppliedHunks {
affected_paths: AffectedPaths {
added,
modified,
deleted,
},
delta: AppliedPatchDelta::new(delta_changes, delta_exact),
})
}
async fn read_optional_file_text_for_delta(
path: &AbsolutePathBuf,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
exact: &mut bool,
) -> Option<String> {
note_existing_path_delta_support(path, fs, sandbox, exact).await;
match fs.read_file_text(path, sandbox).await {
Ok(content) => Some(content),
Err(source) if source.kind() == io::ErrorKind::NotFound => None,
Err(_) => {
*exact = false;
None
}
}
}
async fn note_existing_path_delta_support(
path: &AbsolutePathBuf,
fs: &dyn ExecutorFileSystem,
sandbox: Option<&FileSystemSandboxContext>,
exact: &mut bool,
) {
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 => {}
Err(_) => *exact = false,
}
}
async fn write_file_with_missing_parent_retry(
fs: &dyn ExecutorFileSystem,
path_abs: &AbsolutePathBuf,
@@ -561,6 +690,7 @@ fn apply_replacements(
#[derive(Debug, Eq, PartialEq)]
pub struct ApplyPatchFileUpdate {
unified_diff: String,
original_content: String,
content: String,
}
@@ -588,6 +718,7 @@ pub async fn unified_diff_from_chunks_with_context(
let unified_diff = text_diff.unified_diff().context_radius(context).to_string();
Ok(ApplyPatchFileUpdate {
unified_diff,
original_content: original_contents,
content: new_contents,
})
}
@@ -1082,6 +1213,7 @@ mod tests {
"#;
let expected = ApplyPatchFileUpdate {
unified_diff: expected_diff.to_string(),
original_content: "foo\nbar\nbaz\nqux\n".to_string(),
content: "foo\nBAR\nbaz\nQUX\n".to_string(),
};
assert_eq!(expected, diff);
@@ -1122,6 +1254,7 @@ mod tests {
"#;
let expected = ApplyPatchFileUpdate {
unified_diff: expected_diff.to_string(),
original_content: "foo\nbar\nbaz\n".to_string(),
content: "FOO\nbar\nbaz\n".to_string(),
};
assert_eq!(expected, diff);
@@ -1163,6 +1296,7 @@ mod tests {
"#;
let expected = ApplyPatchFileUpdate {
unified_diff: expected_diff.to_string(),
original_content: "foo\nbar\nbaz\n".to_string(),
content: "foo\nbar\nBAZ\n".to_string(),
};
assert_eq!(expected, diff);
@@ -1201,6 +1335,7 @@ mod tests {
"#;
let expected = ApplyPatchFileUpdate {
unified_diff: expected_diff.to_string(),
original_content: "foo\nbar\nbaz\n".to_string(),
content: "foo\nbar\nbaz\nquux\n".to_string(),
};
assert_eq!(expected, diff);
@@ -1260,6 +1395,7 @@ mod tests {
let expected = ApplyPatchFileUpdate {
unified_diff: expected_diff.to_string(),
original_content: "a\nb\nc\nd\ne\nf\n".to_string(),
content: "a\nB\nc\nd\nE\nf\ng\n".to_string(),
};
@@ -1318,4 +1454,59 @@ g
.await;
assert!(result.is_err());
}
#[tokio::test]
async fn test_unreadable_destinations_return_inexact_delta() {
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();
for patch in [
wrap_patch("*** Add File: binary.dat\n+text"),
wrap_patch("*** Update File: source.txt\n*** Move to: binary.dat\n@@\n-before\n+after"),
] {
fs::write(&path, [0xff, 0xfe, 0xfd]).unwrap();
let mut stdout = Vec::new();
let mut stderr = Vec::new();
let delta = apply_patch(
&patch,
&cwd,
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
.await
.unwrap();
assert!(!delta.is_exact());
}
}
#[cfg(unix)]
#[tokio::test]
async fn test_delete_symlink_returns_inexact_delta() {
use std::os::unix::fs::symlink;
let dir = tempdir().unwrap();
fs::write(dir.path().join("target.txt"), "target\n").unwrap();
symlink(dir.path().join("target.txt"), dir.path().join("link.txt")).unwrap();
let patch = wrap_patch("*** Delete File: link.txt");
let mut stdout = Vec::new();
let mut stderr = Vec::new();
let delta = apply_patch(
&patch,
&AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(),
&mut stdout,
&mut stderr,
LOCAL_FS.as_ref(),
/*sandbox*/ None,
)
.await
.unwrap();
assert!(!delta.is_exact());
}
}
codex-rs/apply-patch/src/standalone_executable.rs
+1 -1
View File
@@ -73,7 +73,7 @@ pub fn run_main() -> i32 {
codex_exec_server::LOCAL_FS.as_ref(),
/*sandbox*/ None,
)) {
Ok(()) => {
Ok(_) => {
// Flush to ensure output ordering when used in pipelines.
let _ = stdout.flush();
0