From 1e832b1438631cda9b3d6fa8f0ae955d7af7813e Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Mon, 24 Nov 2025 11:32:47 -0800 Subject: [PATCH] fix(windows) support apply_patch parsing in powershell (#7221) ## Summary Support powershell parsing of apply_patch ## Testing - [x] Enable apply_patch unit tests --------- Co-authored-by: jif-oai --- .../app-server/tests/suite/v2/turn_start.rs | 8 - codex-rs/apply-patch/src/lib.rs | 143 ++++++++++++++---- codex-rs/core/tests/suite/exec_policy.rs | 2 +- 3 files changed, 115 insertions(+), 38 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index de4d1cd2c..a3e75268c 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -614,10 +614,6 @@ async fn turn_start_updates_sandbox_and_cwd_between_turns_v2() -> Result<()> { #[tokio::test] async fn turn_start_file_change_approval_v2() -> Result<()> { skip_if_no_network!(Ok(())); - if cfg!(windows) { - // TODO apply_patch approvals are not parsed from powershell commands yet - return Ok(()); - } let tmp = TempDir::new()?; let codex_home = tmp.path().join("codex_home"); @@ -764,10 +760,6 @@ async fn turn_start_file_change_approval_v2() -> Result<()> { #[tokio::test] async fn turn_start_file_change_approval_decline_v2() -> Result<()> { skip_if_no_network!(Ok(())); - if cfg!(windows) { - // TODO apply_patch approvals are not parsed from powershell commands yet - return Ok(()); - } let tmp = TempDir::new()?; let codex_home = tmp.path().join("codex_home"); diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index a2bea0514..867d19a2e 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -30,7 +30,13 @@ pub use standalone_executable::main; pub const APPLY_PATCH_TOOL_INSTRUCTIONS: &str = include_str!("../apply_patch_tool_instructions.md"); const APPLY_PATCH_COMMANDS: [&str; 2] = ["apply_patch", "applypatch"]; -const APPLY_PATCH_SHELLS: [&str; 3] = ["bash", "zsh", "sh"]; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ApplyPatchShell { + Unix, + PowerShell, + Cmd, +} #[derive(Debug, Error, PartialEq)] pub enum ApplyPatchError { @@ -97,11 +103,55 @@ pub struct ApplyPatchArgs { pub workdir: Option, } -fn shell_supports_apply_patch(shell: &str) -> bool { +fn classify_shell_name(shell: &str) -> Option { std::path::Path::new(shell) - .file_name() + .file_stem() .and_then(|name| name.to_str()) - .is_some_and(|name| APPLY_PATCH_SHELLS.contains(&name)) + .map(str::to_ascii_lowercase) +} + +fn classify_shell(shell: &str, flag: &str) -> Option { + classify_shell_name(shell).and_then(|name| match name.as_str() { + "bash" | "zsh" | "sh" if flag == "-lc" => Some(ApplyPatchShell::Unix), + "pwsh" | "powershell" if flag.eq_ignore_ascii_case("-command") => { + Some(ApplyPatchShell::PowerShell) + } + "cmd" if flag.eq_ignore_ascii_case("/c") => Some(ApplyPatchShell::Cmd), + _ => None, + }) +} + +fn can_skip_flag(shell: &str, flag: &str) -> bool { + classify_shell_name(shell).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)> { + match argv { + [shell, flag, script] => classify_shell(shell, flag).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| { + let script = script.as_str(); + (shell_type, script) + }) + } + _ => None, + } +} + +fn extract_apply_patch_from_shell( + shell: ApplyPatchShell, + script: &str, +) -> std::result::Result<(String, Option), ExtractHeredocError> { + match shell { + ApplyPatchShell::Unix | ApplyPatchShell::PowerShell | ApplyPatchShell::Cmd => { + extract_apply_patch_from_bash(script) + } + } } pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { @@ -111,9 +161,9 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { Ok(source) => MaybeApplyPatch::Body(source), Err(e) => MaybeApplyPatch::PatchParseError(e), }, - // Bash heredoc form: (optional `cd &&`) apply_patch <<'EOF' ... - [shell, flag, script] if shell_supports_apply_patch(shell) && flag == "-lc" => { - match extract_apply_patch_from_bash(script) { + // Shell heredoc form: (optional `cd &&`) apply_patch <<'EOF' ... + _ => match parse_shell_script(argv) { + Some((shell, script)) => match extract_apply_patch_from_shell(shell, script) { Ok((body, workdir)) => match parse_patch(&body) { Ok(mut source) => { source.workdir = workdir; @@ -125,9 +175,9 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { MaybeApplyPatch::NotApplyPatch } Err(e) => MaybeApplyPatch::ShellParseError(e), - } - } - _ => MaybeApplyPatch::NotApplyPatch, + }, + None => MaybeApplyPatch::NotApplyPatch, + }, } } @@ -222,24 +272,17 @@ impl ApplyPatchAction { /// cwd must be an absolute path so that we can resolve relative paths in the /// patch. 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 + // Detect a raw patch body passed directly as the command or as the body of a shell // 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, - ); - } - } - [shell, flag, script] - if shell_supports_apply_patch(shell) - && flag == "-lc" - && parse_patch(script).is_ok() => - { - return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation); - } - _ => {} + 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) { @@ -871,6 +914,22 @@ mod tests { strs_to_strings(&["bash", "-lc", script]) } + fn args_powershell(script: &str) -> Vec { + strs_to_strings(&["powershell.exe", "-Command", script]) + } + + fn args_powershell_no_profile(script: &str) -> Vec { + strs_to_strings(&["powershell.exe", "-NoProfile", "-Command", script]) + } + + fn args_pwsh(script: &str) -> Vec { + strs_to_strings(&["pwsh", "-NoProfile", "-Command", script]) + } + + fn args_cmd(script: &str) -> Vec { + strs_to_strings(&["cmd.exe", "/c", script]) + } + fn heredoc_script(prefix: &str) -> String { format!( "{prefix}apply_patch <<'PATCH'\n*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch\nPATCH" @@ -890,8 +949,7 @@ mod tests { }] } - fn assert_match(script: &str, expected_workdir: Option<&str>) { - let args = args_bash(script); + fn assert_match_args(args: Vec, expected_workdir: Option<&str>) { match maybe_parse_apply_patch(&args) { MaybeApplyPatch::Body(ApplyPatchArgs { hunks, workdir, .. }) => { assert_eq!(workdir.as_deref(), expected_workdir); @@ -901,6 +959,11 @@ mod tests { } } + fn assert_match(script: &str, expected_workdir: Option<&str>) { + let args = args_bash(script); + assert_match_args(args, expected_workdir); + } + fn assert_not_match(script: &str) { let args = args_bash(script); assert_matches!( @@ -1014,6 +1077,28 @@ PATCH"#, } } + #[test] + fn test_powershell_heredoc() { + let script = heredoc_script(""); + assert_match_args(args_powershell(&script), None); + } + #[test] + fn test_powershell_heredoc_no_profile() { + let script = heredoc_script(""); + assert_match_args(args_powershell_no_profile(&script), None); + } + #[test] + fn test_pwsh_heredoc() { + let script = heredoc_script(""); + assert_match_args(args_pwsh(&script), None); + } + + #[test] + fn test_cmd_heredoc_with_cd() { + let script = heredoc_script("cd foo && "); + assert_match_args(args_cmd(&script), Some("foo")); + } + #[test] fn test_heredoc_with_leading_cd() { assert_match(&heredoc_script("cd foo && "), Some("foo")); diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index 443e37cdb..b65b2994e 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -26,7 +26,7 @@ async fn execpolicy_blocks_shell_invocation() -> Result<()> { return Ok(()); } - let mut builder = test_codex().with_model("gpt-5.1").with_config(|config| { + let mut builder = test_codex().with_config(|config| { let policy_path = config.codex_home.join("policy").join("policy.codexpolicy"); fs::create_dir_all( policy_path