Skip to content

Commit 1e832b1

Browse files
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 <[email protected]>
1 parent c31663d commit 1e832b1

File tree

3 files changed

+115
-38
lines changed

3 files changed

+115
-38
lines changed

codex-rs/app-server/tests/suite/v2/turn_start.rs

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -614,10 +614,6 @@ async fn turn_start_updates_sandbox_and_cwd_between_turns_v2() -> Result<()> {
614614
#[tokio::test]
615615
async fn turn_start_file_change_approval_v2() -> Result<()> {
616616
skip_if_no_network!(Ok(()));
617-
if cfg!(windows) {
618-
// TODO apply_patch approvals are not parsed from powershell commands yet
619-
return Ok(());
620-
}
621617

622618
let tmp = TempDir::new()?;
623619
let codex_home = tmp.path().join("codex_home");
@@ -764,10 +760,6 @@ async fn turn_start_file_change_approval_v2() -> Result<()> {
764760
#[tokio::test]
765761
async fn turn_start_file_change_approval_decline_v2() -> Result<()> {
766762
skip_if_no_network!(Ok(()));
767-
if cfg!(windows) {
768-
// TODO apply_patch approvals are not parsed from powershell commands yet
769-
return Ok(());
770-
}
771763

772764
let tmp = TempDir::new()?;
773765
let codex_home = tmp.path().join("codex_home");

codex-rs/apply-patch/src/lib.rs

Lines changed: 114 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,13 @@ pub use standalone_executable::main;
3030
pub const APPLY_PATCH_TOOL_INSTRUCTIONS: &str = include_str!("../apply_patch_tool_instructions.md");
3131

3232
const APPLY_PATCH_COMMANDS: [&str; 2] = ["apply_patch", "applypatch"];
33-
const APPLY_PATCH_SHELLS: [&str; 3] = ["bash", "zsh", "sh"];
33+
34+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
35+
enum ApplyPatchShell {
36+
Unix,
37+
PowerShell,
38+
Cmd,
39+
}
3440

3541
#[derive(Debug, Error, PartialEq)]
3642
pub enum ApplyPatchError {
@@ -97,11 +103,55 @@ pub struct ApplyPatchArgs {
97103
pub workdir: Option<String>,
98104
}
99105

100-
fn shell_supports_apply_patch(shell: &str) -> bool {
106+
fn classify_shell_name(shell: &str) -> Option<String> {
101107
std::path::Path::new(shell)
102-
.file_name()
108+
.file_stem()
103109
.and_then(|name| name.to_str())
104-
.is_some_and(|name| APPLY_PATCH_SHELLS.contains(&name))
110+
.map(str::to_ascii_lowercase)
111+
}
112+
113+
fn classify_shell(shell: &str, flag: &str) -> Option<ApplyPatchShell> {
114+
classify_shell_name(shell).and_then(|name| match name.as_str() {
115+
"bash" | "zsh" | "sh" if flag == "-lc" => Some(ApplyPatchShell::Unix),
116+
"pwsh" | "powershell" if flag.eq_ignore_ascii_case("-command") => {
117+
Some(ApplyPatchShell::PowerShell)
118+
}
119+
"cmd" if flag.eq_ignore_ascii_case("/c") => Some(ApplyPatchShell::Cmd),
120+
_ => None,
121+
})
122+
}
123+
124+
fn can_skip_flag(shell: &str, flag: &str) -> bool {
125+
classify_shell_name(shell).is_some_and(|name| {
126+
matches!(name.as_str(), "pwsh" | "powershell") && flag.eq_ignore_ascii_case("-noprofile")
127+
})
128+
}
129+
130+
fn parse_shell_script(argv: &[String]) -> Option<(ApplyPatchShell, &str)> {
131+
match argv {
132+
[shell, flag, script] => classify_shell(shell, flag).map(|shell_type| {
133+
let script = script.as_str();
134+
(shell_type, script)
135+
}),
136+
[shell, skip_flag, flag, script] if can_skip_flag(shell, skip_flag) => {
137+
classify_shell(shell, flag).map(|shell_type| {
138+
let script = script.as_str();
139+
(shell_type, script)
140+
})
141+
}
142+
_ => None,
143+
}
144+
}
145+
146+
fn extract_apply_patch_from_shell(
147+
shell: ApplyPatchShell,
148+
script: &str,
149+
) -> std::result::Result<(String, Option<String>), ExtractHeredocError> {
150+
match shell {
151+
ApplyPatchShell::Unix | ApplyPatchShell::PowerShell | ApplyPatchShell::Cmd => {
152+
extract_apply_patch_from_bash(script)
153+
}
154+
}
105155
}
106156

107157
pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch {
@@ -111,9 +161,9 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch {
111161
Ok(source) => MaybeApplyPatch::Body(source),
112162
Err(e) => MaybeApplyPatch::PatchParseError(e),
113163
},
114-
// Bash heredoc form: (optional `cd <path> &&`) apply_patch <<'EOF' ...
115-
[shell, flag, script] if shell_supports_apply_patch(shell) && flag == "-lc" => {
116-
match extract_apply_patch_from_bash(script) {
164+
// Shell heredoc form: (optional `cd <path> &&`) apply_patch <<'EOF' ...
165+
_ => match parse_shell_script(argv) {
166+
Some((shell, script)) => match extract_apply_patch_from_shell(shell, script) {
117167
Ok((body, workdir)) => match parse_patch(&body) {
118168
Ok(mut source) => {
119169
source.workdir = workdir;
@@ -125,9 +175,9 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch {
125175
MaybeApplyPatch::NotApplyPatch
126176
}
127177
Err(e) => MaybeApplyPatch::ShellParseError(e),
128-
}
129-
}
130-
_ => MaybeApplyPatch::NotApplyPatch,
178+
},
179+
None => MaybeApplyPatch::NotApplyPatch,
180+
},
131181
}
132182
}
133183

@@ -222,24 +272,17 @@ impl ApplyPatchAction {
222272
/// cwd must be an absolute path so that we can resolve relative paths in the
223273
/// patch.
224274
pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApplyPatchVerified {
225-
// Detect a raw patch body passed directly as the command or as the body of a bash -lc
275+
// Detect a raw patch body passed directly as the command or as the body of a shell
226276
// script. In these cases, report an explicit error rather than applying the patch.
227-
match argv {
228-
[body] => {
229-
if parse_patch(body).is_ok() {
230-
return MaybeApplyPatchVerified::CorrectnessError(
231-
ApplyPatchError::ImplicitInvocation,
232-
);
233-
}
234-
}
235-
[shell, flag, script]
236-
if shell_supports_apply_patch(shell)
237-
&& flag == "-lc"
238-
&& parse_patch(script).is_ok() =>
239-
{
240-
return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation);
241-
}
242-
_ => {}
277+
if let [body] = argv
278+
&& parse_patch(body).is_ok()
279+
{
280+
return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation);
281+
}
282+
if let Some((_, script)) = parse_shell_script(argv)
283+
&& parse_patch(script).is_ok()
284+
{
285+
return MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation);
243286
}
244287

245288
match maybe_parse_apply_patch(argv) {
@@ -871,6 +914,22 @@ mod tests {
871914
strs_to_strings(&["bash", "-lc", script])
872915
}
873916

917+
fn args_powershell(script: &str) -> Vec<String> {
918+
strs_to_strings(&["powershell.exe", "-Command", script])
919+
}
920+
921+
fn args_powershell_no_profile(script: &str) -> Vec<String> {
922+
strs_to_strings(&["powershell.exe", "-NoProfile", "-Command", script])
923+
}
924+
925+
fn args_pwsh(script: &str) -> Vec<String> {
926+
strs_to_strings(&["pwsh", "-NoProfile", "-Command", script])
927+
}
928+
929+
fn args_cmd(script: &str) -> Vec<String> {
930+
strs_to_strings(&["cmd.exe", "/c", script])
931+
}
932+
874933
fn heredoc_script(prefix: &str) -> String {
875934
format!(
876935
"{prefix}apply_patch <<'PATCH'\n*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch\nPATCH"
@@ -890,8 +949,7 @@ mod tests {
890949
}]
891950
}
892951

893-
fn assert_match(script: &str, expected_workdir: Option<&str>) {
894-
let args = args_bash(script);
952+
fn assert_match_args(args: Vec<String>, expected_workdir: Option<&str>) {
895953
match maybe_parse_apply_patch(&args) {
896954
MaybeApplyPatch::Body(ApplyPatchArgs { hunks, workdir, .. }) => {
897955
assert_eq!(workdir.as_deref(), expected_workdir);
@@ -901,6 +959,11 @@ mod tests {
901959
}
902960
}
903961

962+
fn assert_match(script: &str, expected_workdir: Option<&str>) {
963+
let args = args_bash(script);
964+
assert_match_args(args, expected_workdir);
965+
}
966+
904967
fn assert_not_match(script: &str) {
905968
let args = args_bash(script);
906969
assert_matches!(
@@ -1014,6 +1077,28 @@ PATCH"#,
10141077
}
10151078
}
10161079

1080+
#[test]
1081+
fn test_powershell_heredoc() {
1082+
let script = heredoc_script("");
1083+
assert_match_args(args_powershell(&script), None);
1084+
}
1085+
#[test]
1086+
fn test_powershell_heredoc_no_profile() {
1087+
let script = heredoc_script("");
1088+
assert_match_args(args_powershell_no_profile(&script), None);
1089+
}
1090+
#[test]
1091+
fn test_pwsh_heredoc() {
1092+
let script = heredoc_script("");
1093+
assert_match_args(args_pwsh(&script), None);
1094+
}
1095+
1096+
#[test]
1097+
fn test_cmd_heredoc_with_cd() {
1098+
let script = heredoc_script("cd foo && ");
1099+
assert_match_args(args_cmd(&script), Some("foo"));
1100+
}
1101+
10171102
#[test]
10181103
fn test_heredoc_with_leading_cd() {
10191104
assert_match(&heredoc_script("cd foo && "), Some("foo"));

codex-rs/core/tests/suite/exec_policy.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ async fn execpolicy_blocks_shell_invocation() -> Result<()> {
2626
return Ok(());
2727
}
2828

29-
let mut builder = test_codex().with_model("gpt-5.1").with_config(|config| {
29+
let mut builder = test_codex().with_config(|config| {
3030
let policy_path = config.codex_home.join("policy").join("policy.codexpolicy");
3131
fs::create_dir_all(
3232
policy_path

0 commit comments

Comments
 (0)