if a command parses as a patch, do not attempt to run it (#3382)
sometimes the model forgets to actually invoke `apply_patch` and puts a
patch as the script body. trying to execute this as bash sometimes
creates files named `,` or `{` or does other unknown things, so catch
this situation and return an error to the model.
This commit is contained in:
@@ -40,6 +40,11 @@ pub enum ApplyPatchError {
|
|||||||
/// Error that occurs while computing replacements when applying patch chunks
|
/// Error that occurs while computing replacements when applying patch chunks
|
||||||
#[error("{0}")]
|
#[error("{0}")]
|
||||||
ComputeReplacements(String),
|
ComputeReplacements(String),
|
||||||
|
/// 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>\"]"
|
||||||
|
)]
|
||||||
|
ImplicitInvocation,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl From<std::io::Error> for ApplyPatchError {
|
impl From<std::io::Error> for ApplyPatchError {
|
||||||
@@ -93,10 +98,12 @@ pub struct ApplyPatchArgs {
|
|||||||
|
|
||||||
pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch {
|
pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch {
|
||||||
match argv {
|
match argv {
|
||||||
|
// Direct invocation: apply_patch <patch>
|
||||||
[cmd, body] if APPLY_PATCH_COMMANDS.contains(&cmd.as_str()) => match parse_patch(body) {
|
[cmd, body] if APPLY_PATCH_COMMANDS.contains(&cmd.as_str()) => match parse_patch(body) {
|
||||||
Ok(source) => MaybeApplyPatch::Body(source),
|
Ok(source) => MaybeApplyPatch::Body(source),
|
||||||
Err(e) => MaybeApplyPatch::PatchParseError(e),
|
Err(e) => MaybeApplyPatch::PatchParseError(e),
|
||||||
},
|
},
|
||||||
|
// Bash heredoc form: (optional `cd <path> &&`) apply_patch <<'EOF' ...
|
||||||
[bash, flag, script] if bash == "bash" && flag == "-lc" => {
|
[bash, flag, script] if bash == "bash" && flag == "-lc" => {
|
||||||
match extract_apply_patch_from_bash(script) {
|
match extract_apply_patch_from_bash(script) {
|
||||||
Ok((body, workdir)) => match parse_patch(&body) {
|
Ok((body, workdir)) => match parse_patch(&body) {
|
||||||
@@ -207,6 +214,26 @@ impl ApplyPatchAction {
|
|||||||
/// cwd must be an absolute path so that we can resolve relative paths in the
|
/// cwd must be an absolute path so that we can resolve relative paths in the
|
||||||
/// patch.
|
/// patch.
|
||||||
pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApplyPatchVerified {
|
pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApplyPatchVerified {
|
||||||
|
// Detect a raw patch body passed directly as the command or as the body of a bash -lc
|
||||||
|
// script. In these cases, report an explicit error rather than applying the patch.
|
||||||
|
match argv {
|
||||||
|
[body] => {
|
||||||
|
if parse_patch(body).is_ok() {
|
||||||
|
return MaybeApplyPatchVerified::CorrectnessError(
|
||||||
|
ApplyPatchError::ImplicitInvocation,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
[bash, flag, script] if bash == "bash" && flag == "-lc" => {
|
||||||
|
if parse_patch(script).is_ok() {
|
||||||
|
return MaybeApplyPatchVerified::CorrectnessError(
|
||||||
|
ApplyPatchError::ImplicitInvocation,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
_ => {}
|
||||||
|
}
|
||||||
|
|
||||||
match maybe_parse_apply_patch(argv) {
|
match maybe_parse_apply_patch(argv) {
|
||||||
MaybeApplyPatch::Body(ApplyPatchArgs {
|
MaybeApplyPatch::Body(ApplyPatchArgs {
|
||||||
patch,
|
patch,
|
||||||
@@ -875,6 +902,28 @@ mod tests {
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_implicit_patch_single_arg_is_error() {
|
||||||
|
let patch = "*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch".to_string();
|
||||||
|
let args = vec![patch];
|
||||||
|
let dir = tempdir().unwrap();
|
||||||
|
assert!(matches!(
|
||||||
|
maybe_parse_apply_patch_verified(&args, dir.path()),
|
||||||
|
MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation)
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_implicit_patch_bash_script_is_error() {
|
||||||
|
let script = "*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch";
|
||||||
|
let args = args_bash(script);
|
||||||
|
let dir = tempdir().unwrap();
|
||||||
|
assert!(matches!(
|
||||||
|
maybe_parse_apply_patch_verified(&args, dir.path()),
|
||||||
|
MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation)
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_literal() {
|
fn test_literal() {
|
||||||
let args = strs_to_strings(&[
|
let args = strs_to_strings(&[
|
||||||
|
|||||||
Reference in New Issue
Block a user