fix: consolidate logic related to checking list of safe commands

I noted that the logic for determining whether an `rg` or `git` invocation was
safe diverged between `is_safe_command.rs` and `windows_safe_commands.rs`, so
this PR reconciles the two in a shared `shared_rules.rs` file.

codex-rs/core/src/command_safety/is_safe_command.rs

@@ -1,4 +1,6 @@
 use crate::bash::parse_shell_lc_plain_commands;
+use crate::command_safety::shared_rules::is_safe_git_command;
+use crate::command_safety::shared_rules::is_safe_ripgrep_command;
 use crate::command_safety::windows_safe_commands::is_safe_command_windows;
 
 pub fn is_known_safe_command(command: &[String]) -> bool {
@@ -108,33 +110,10 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
         }
 
         // Ripgrep
-        Some("rg") => {
-            const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &[
-                // Takes an arbitrary command that is executed for each match.
-                "--pre",
-                // Takes a command that can be used to obtain the local hostname.
-                "--hostname-bin",
-            ];
-            const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &[
-                // Calls out to other decompression tools, so do not auto-approve
-                // out of an abundance of caution.
-                "--search-zip",
-                "-z",
-            ];
-
-            !command.iter().any(|arg| {
-                UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS.contains(&arg.as_str())
-                    || UNSAFE_RIPGREP_OPTIONS_WITH_ARGS
-                        .iter()
-                        .any(|&opt| arg == opt || arg.starts_with(&format!("{opt}=")))
-            })
-        }
+        Some("rg") => is_safe_ripgrep_command(command),
 
         // Git
-        Some("git") => matches!(
-            command.get(1).map(String::as_str),
-            Some("branch" | "status" | "log" | "diff" | "show")
-        ),
+        Some("git") => is_safe_git_command(command),
 
         // Rust
         Some("cargo") if command.get(1).map(String::as_str) == Some("check") => true,
@@ -280,40 +259,6 @@ mod tests {
         }
     }
 
-    #[test]
-    fn ripgrep_rules() {
-        // Safe ripgrep invocations – none of the unsafe flags are present.
-        assert!(is_safe_to_call_with_exec(&vec_str(&[
-            "rg",
-            "Cargo.toml",
-            "-n"
-        ])));
-
-        // Unsafe flags that do not take an argument (present verbatim).
-        for args in [
-            vec_str(&["rg", "--search-zip", "files"]),
-            vec_str(&["rg", "-z", "files"]),
-        ] {
-            assert!(
-                !is_safe_to_call_with_exec(&args),
-                "expected {args:?} to be considered unsafe due to zip-search flag",
-            );
-        }
-
-        // Unsafe flags that expect a value, provided in both split and = forms.
-        for args in [
-            vec_str(&["rg", "--pre", "pwned", "files"]),
-            vec_str(&["rg", "--pre=pwned", "files"]),
-            vec_str(&["rg", "--hostname-bin", "pwned", "files"]),
-            vec_str(&["rg", "--hostname-bin=pwned", "files"]),
-        ] {
-            assert!(
-                !is_safe_to_call_with_exec(&args),
-                "expected {args:?} to be considered unsafe due to external-command flag",
-            );
-        }
-    }
-
     #[test]
     fn windows_powershell_full_path_is_safe() {
         if !cfg!(windows) {

codex-rs/core/src/command_safety/mod.rs

@@ -1,3 +1,4 @@
 pub mod is_dangerous_command;
 pub mod is_safe_command;
+mod shared_rules;
 pub mod windows_safe_commands;

codex-rs/core/src/command_safety/shared_rules.rs

@@ -0,0 +1,138 @@
+//! Shared safelist logic for commands that are allowed on both Unix and Windows.
+//! Keep these helpers read-only and conservative.
+
+/// Ensures the ripgrep invocation does not contain flags that could run
+/// arbitrary commands.
+pub(crate) fn is_safe_ripgrep_command<S: AsRef<str>>(words: &[S]) -> bool {
+    const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &[
+        // Takes an arbitrary command that is executed for each match.
+        "--pre",
+        // Takes a command that can be used to obtain the local hostname.
+        "--hostname-bin",
+    ];
+    const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &[
+        // Calls out to other decompression tools, so do not auto-approve out of
+        // an abundance of caution.
+        "--search-zip",
+        "-z",
+    ];
+
+    !words.iter().skip(1).any(|arg| {
+        let arg_lc = arg.as_ref().to_ascii_lowercase();
+        UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS.contains(&arg_lc.as_str())
+            || UNSAFE_RIPGREP_OPTIONS_WITH_ARGS
+                .iter()
+                .any(|opt| arg_lc == *opt || arg_lc.starts_with(&format!("{opt}=")))
+    })
+}
+
+/// Ensures a Git command sticks to whitelisted read-only subcommands and flags.
+pub(crate) fn is_safe_git_command<S: AsRef<str>>(words: &[S]) -> bool {
+    const SAFE_SUBCOMMANDS: &[&str] = &["status", "log", "show", "diff", "cat-file"];
+
+    let mut iter = words.iter().skip(1);
+    while let Some(arg) = iter.next() {
+        let arg = arg.as_ref();
+        let arg_lc = arg.to_ascii_lowercase();
+
+        if arg.starts_with('-') {
+            if arg_lc.starts_with("-c=") || arg_lc.starts_with("--config=") {
+                return false;
+            }
+
+            if arg_lc.starts_with("--git-dir=") || arg_lc.starts_with("--work-tree=") {
+                continue;
+            }
+
+            if arg.eq_ignore_ascii_case("-c") || arg.eq_ignore_ascii_case("--config") {
+                return false;
+            }
+
+            if arg.eq_ignore_ascii_case("--git-dir") || arg.eq_ignore_ascii_case("--work-tree") {
+                if iter.next().is_none() {
+                    return false;
+                }
+                continue;
+            }
+
+            continue;
+        }
+
+        return SAFE_SUBCOMMANDS.contains(&arg_lc.as_str());
+    }
+
+    false
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    const RIPGREP_UNSAFE_NO_ARG: &[&[&str]] =
+        &[&["rg", "--search-zip", "files"], &["rg", "-z", "files"]];
+    const RIPGREP_UNSAFE_WITH_ARG: &[&[&str]] = &[
+        &["rg", "--pre", "pwned", "files"],
+        &["rg", "--pre=pwned", "files"],
+        &["rg", "--hostname-bin", "pwned", "files"],
+        &["rg", "--hostname-bin=pwned", "files"],
+    ];
+
+    #[test]
+    fn ripgrep_rules() {
+        // Safe ripgrep invocations – none of the unsafe flags are present.
+        assert!(is_safe_ripgrep_command(&["rg", "Cargo.toml", "-n"]));
+
+        // Unsafe flags that do not take an argument (present verbatim).
+        for args in RIPGREP_UNSAFE_NO_ARG {
+            assert!(
+                !is_safe_ripgrep_command(args),
+                "expected {args:?} to be considered unsafe due to zip-search flag",
+            );
+        }
+
+        // Unsafe flags that expect a value, provided in both split and = forms.
+        for args in RIPGREP_UNSAFE_WITH_ARG {
+            assert!(
+                !is_safe_ripgrep_command(args),
+                "expected {args:?} to be considered unsafe due to external-command flag",
+            );
+        }
+    }
+
+    #[test]
+    fn git_rules() {
+        assert!(is_safe_git_command(&["git", "status"]));
+        assert!(is_safe_git_command(&[
+            "git",
+            "--work-tree=.",
+            "--git-dir=.git",
+            "diff"
+        ]));
+
+        assert!(!is_safe_git_command(&["git"]));
+        assert!(!is_safe_git_command(&["git", "branch"]));
+        assert!(!is_safe_git_command(&["git", "fetch"]));
+        assert!(!is_safe_git_command(&["git", "-c"]));
+        assert!(!is_safe_git_command(&[
+            "git",
+            "-c",
+            "core.pager=cat",
+            "show"
+        ]));
+        assert!(!is_safe_git_command(&[
+            "git",
+            "--config",
+            "core.pager=cat",
+            "show"
+        ]));
+        assert!(!is_safe_git_command(&["git", "-c=core.pager=cat", "show"]));
+        assert!(!is_safe_git_command(&[
+            "git",
+            "--config=core.pager=cat",
+            "show"
+        ]));
+        assert!(!is_safe_git_command(&["git", "--config"]));
+        assert!(!is_safe_git_command(&["git", "--git-dir"]));
+        assert!(!is_safe_git_command(&["git", "--work-tree"]));
+    }
+}

codex-rs/core/src/command_safety/windows_safe_commands.rs

@@ -5,6 +5,9 @@ use std::path::Path;
 use std::process::Command;
 use std::sync::LazyLock;
 
+use crate::command_safety::shared_rules::is_safe_git_command;
+use crate::command_safety::shared_rules::is_safe_ripgrep_command;
+
 const POWERSHELL_PARSER_SCRIPT: &str = include_str!("powershell_parser.ps1");
 
 /// On Windows, we conservatively allow only clearly read-only PowerShell invocations
@@ -270,7 +273,7 @@ fn is_safe_powershell_command(words: &[String]) -> bool {
 
         "git" => is_safe_git_command(words),
 
-        "rg" => is_safe_ripgrep(words),
+        "rg" => is_safe_ripgrep_command(words),
 
         // Extra safety: explicitly prohibit common side-effecting cmdlets regardless of args.
         "set-content" | "add-content" | "out-file" | "new-item" | "remove-item" | "move-item"
@@ -286,64 +289,6 @@ fn is_safe_powershell_command(words: &[String]) -> bool {
     }
 }
 
-/// Checks that an `rg` invocation avoids options that can spawn arbitrary executables.
-fn is_safe_ripgrep(words: &[String]) -> bool {
-    const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &["--pre", "--hostname-bin"];
-    const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &["--search-zip", "-z"];
-
-    !words.iter().skip(1).any(|arg| {
-        let arg_lc = arg.to_ascii_lowercase();
-        // Examples rejected here: "pwsh -Command 'rg --pre cat pattern'" and "pwsh -Command 'rg --search-zip pattern'".
-        UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS.contains(&arg_lc.as_str())
-            || UNSAFE_RIPGREP_OPTIONS_WITH_ARGS
-                .iter()
-                .any(|opt| arg_lc == *opt || arg_lc.starts_with(&format!("{opt}=")))
-    })
-}
-
-/// Ensures a Git command sticks to whitelisted read-only subcommands and flags.
-fn is_safe_git_command(words: &[String]) -> bool {
-    const SAFE_SUBCOMMANDS: &[&str] = &["status", "log", "show", "diff", "cat-file"];
-
-    let mut iter = words.iter().skip(1);
-    while let Some(arg) = iter.next() {
-        let arg_lc = arg.to_ascii_lowercase();
-
-        if arg.starts_with('-') {
-            if arg.eq_ignore_ascii_case("-c") || arg.eq_ignore_ascii_case("--config") {
-                if iter.next().is_none() {
-                    // Examples rejected here: "pwsh -Command 'git -c'" and "pwsh -Command 'git --config'".
-                    return false;
-                }
-                continue;
-            }
-
-            if arg_lc.starts_with("-c=")
-                || arg_lc.starts_with("--config=")
-                || arg_lc.starts_with("--git-dir=")
-                || arg_lc.starts_with("--work-tree=")
-            {
-                continue;
-            }
-
-            if arg.eq_ignore_ascii_case("--git-dir") || arg.eq_ignore_ascii_case("--work-tree") {
-                if iter.next().is_none() {
-                    // Examples rejected here: "pwsh -Command 'git --git-dir'" and "pwsh -Command 'git --work-tree'".
-                    return false;
-                }
-                continue;
-            }
-
-            continue;
-        }
-
-        return SAFE_SUBCOMMANDS.contains(&arg_lc.as_str());
-    }
-
-    // Examples rejected here: "pwsh -Command 'git'" and "pwsh -Command 'git status --short | Remove-Item foo'".
-    false
-}
-
 #[cfg(all(test, windows))]
 mod tests {
     use super::*;