Propagate apply_patch filesystem errors (#1892)
## Summary We have been returning `exit code 0` from the apply patch command when writes fail, which causes our `exec` harness to pass back confusing messages to the model. Instead, we should loudly fail so that the harness and the model can handle these errors appropriately. Also adds a test to confirm this behavior. ## Testing - `cargo test -p codex-apply-patch`
This commit is contained in:
@@ -42,6 +42,15 @@ impl From<std::io::Error> for ApplyPatchError {
|
||||
}
|
||||
}
|
||||
|
||||
impl From<&std::io::Error> for ApplyPatchError {
|
||||
fn from(err: &std::io::Error) -> Self {
|
||||
ApplyPatchError::IoError(IoError {
|
||||
context: "I/O error".to_string(),
|
||||
source: std::io::Error::new(err.kind(), err.to_string()),
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Error)]
|
||||
#[error("{context}: {source}")]
|
||||
pub struct IoError {
|
||||
@@ -366,13 +375,21 @@ pub fn apply_hunks(
|
||||
match apply_hunks_to_files(hunks) {
|
||||
Ok(affected) => {
|
||||
print_summary(&affected, stdout).map_err(ApplyPatchError::from)?;
|
||||
Ok(())
|
||||
}
|
||||
Err(err) => {
|
||||
writeln!(stderr, "{err:?}").map_err(ApplyPatchError::from)?;
|
||||
let msg = err.to_string();
|
||||
writeln!(stderr, "{msg}").map_err(ApplyPatchError::from)?;
|
||||
if let Some(io) = err.downcast_ref::<std::io::Error>() {
|
||||
Err(ApplyPatchError::from(io))
|
||||
} else {
|
||||
Err(ApplyPatchError::IoError(IoError {
|
||||
context: msg,
|
||||
source: std::io::Error::other(err),
|
||||
}))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Applies each parsed patch hunk to the filesystem.
|
||||
@@ -1238,4 +1255,24 @@ g
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_apply_patch_fails_on_write_error() {
|
||||
let dir = tempdir().unwrap();
|
||||
let path = dir.path().join("readonly.txt");
|
||||
fs::write(&path, "before\n").unwrap();
|
||||
let mut perms = fs::metadata(&path).unwrap().permissions();
|
||||
perms.set_readonly(true);
|
||||
fs::set_permissions(&path, perms).unwrap();
|
||||
|
||||
let patch = wrap_patch(&format!(
|
||||
"*** Update File: {}\n@@\n-before\n+after\n*** End Patch",
|
||||
path.display()
|
||||
));
|
||||
|
||||
let mut stdout = Vec::new();
|
||||
let mut stderr = Vec::new();
|
||||
let result = apply_patch(&patch, &mut stdout, &mut stderr);
|
||||
assert!(result.is_err());
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user