git-utils: close merge-driver apply races

codex-rs/git-utils/src/apply.rs

@@ -90,13 +90,14 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result<ApplyGitResult> {
         });
     }
 
-    // Avoid three-way machinery entirely when the patch applies cleanly.
-    // A selected merge driver is relevant only to the three-way fallback.
-    let mut plain_check_args = vec![
-        "apply".to_string(),
-        "--check".to_string(),
-        "--index".to_string(),
-    ];
+    // Avoid three-way machinery entirely when the patch applies cleanly. A
+    // reverse check must inspect the working tree before staging: requiring
+    // the old index to match would misclassify a clean undo as a three-way
+    // fallback. Forward direct application still requires index agreement.
+    let mut plain_check_args = vec!["apply".to_string(), "--check".to_string()];
+    if !req.revert {
+        plain_check_args.push("--index".to_string());
+    }
     if req.revert {
         plain_check_args.push("-R".to_string());
     }
@@ -104,15 +105,21 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result<ApplyGitResult> {
     let (plain_check_code, _, _) = run_git(&git, &git_root, &cfg_parts, &plain_check_args)?;
 
     let mut args = vec!["apply".to_string()];
-    if plain_check_code != 0 {
-        ensure_no_selected_merge_drivers(&git, &git_root, &patch_paths, &cfg_parts)?;
-        if req.revert {
-            // Stage WT paths first to avoid index mismatch on three-way revert.
-            stage_effective_paths(&git, &git_root, &patch_paths, &cfg_parts)?;
+    let merge_guard = if plain_check_code != 0 {
+        let guard = ensure_no_selected_merge_drivers(&git, &git_root, &patch_paths, &cfg_parts)?;
+        if let Some(guard) = &guard {
+            cfg_parts.extend_from_slice(guard.git_config_args());
         }
         args.push("--3way".to_string());
+        guard
     } else {
         args.push("--index".to_string());
+        None
+    };
+    if req.revert {
+        // Stage only after the worktree-only applicability decision. The
+        // guarded config is shared with the final Git command.
+        stage_effective_paths(&git, &git_root, &patch_paths, &cfg_parts)?;
     }
     if req.revert {
         args.push("-R".to_string());
@@ -121,6 +128,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result<ApplyGitResult> {
 
     let cmd_for_log = render_command_for_log(&git_root, &cfg_parts, &args);
     let (code, stdout, stderr) = run_git(&git, &git_root, &cfg_parts, &args)?;
+    drop(merge_guard);
 
     let (mut applied_paths, mut skipped_paths, mut conflicted_paths) =
         parse_git_apply_output(&stdout, &stderr);

codex-rs/git-utils/src/merge_driver.rs

@@ -8,6 +8,7 @@ use std::process::Stdio;
 
 use crate::git_command::GitRunner;
 use crate::git_config::GitConfigEntry;
+use crate::safe_git::GitConfigOverrideFile;
 #[cfg(test)]
 use crate::safe_git::isolate_git_command_environment;
 use crate::safe_git::read_effective_config_with_fallback;
@@ -19,7 +20,7 @@ pub(crate) fn ensure_no_selected_merge_drivers(
     cwd: &Path,
     paths: &[String],
     git_config_args: &[String],
-) -> io::Result<()> {
+) -> io::Result<Option<GitConfigOverrideFile>> {
     let entries = read_merge_config(git, cwd, git_config_args)?;
     let attributes = read_merge_attributes(git, cwd, paths, git_config_args)?;
     if let Some((driver, path)) = untrusted_driver_selection(&entries, &attributes)? {
@@ -30,7 +31,27 @@ pub(crate) fn ensure_no_selected_merge_drivers(
             ),
         ));
     }
-    Ok(())
+
+    let driver_keys = entries
+        .values()
+        .filter(|entry| entry.key != "merge.default" && !entry.value.is_empty())
+        .map(|entry| entry.key.clone())
+        .collect::<Vec<_>>();
+    if driver_keys.is_empty() {
+        return Ok(None);
+    }
+
+    let guard = GitConfigOverrideFile::new("merge-driver-neutralization.gitconfig")?;
+    for key in driver_keys {
+        guard.add_value(
+            git,
+            cwd,
+            &key,
+            "",
+            &format!("Git merge-driver neutralization for {key:?}"),
+        )?;
+    }
+    Ok(Some(guard))
 }
 
 fn read_merge_config(
@@ -176,3 +197,7 @@ fn invalid_config_entry(message: &str) -> io::Error {
 #[cfg(test)]
 #[path = "merge_driver_tests.rs"]
 mod tests;
+
+#[cfg(test)]
+#[path = "merge_driver_race_tests.rs"]
+mod race_tests;

codex-rs/git-utils/src/merge_driver_race_tests.rs

@@ -0,0 +1,307 @@
+use super::*;
+use crate::apply::ApplyGitRequest;
+use crate::apply::apply_git_patch;
+use pretty_assertions::assert_eq;
+use std::fs::OpenOptions;
+use std::path::Path;
+use std::path::PathBuf;
+use std::thread;
+use std::time::Duration;
+use std::time::Instant;
+
+const OLD_DRIVER_COMMAND: &str = "git config codex.oldmergeran true && false";
+const NEW_DRIVER_COMMAND: &str = "git config codex.newmergeran true && false";
+
+fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) {
+    let mut command = std::process::Command::new("git");
+    isolate_git_command_environment(&mut command);
+    let output = command
+        .args(args)
+        .current_dir(cwd)
+        .output()
+        .expect("run Git");
+    (
+        output.status.code().unwrap_or(-1),
+        String::from_utf8_lossy(&output.stdout).into_owned(),
+        String::from_utf8_lossy(&output.stderr).into_owned(),
+    )
+}
+
+fn run_success(cwd: &Path, args: &[&str]) -> String {
+    let (code, stdout, stderr) = run(cwd, args);
+    assert_eq!(code, 0, "git {args:?}: {stderr}");
+    stdout
+}
+
+fn init_repo() -> tempfile::TempDir {
+    let repo = tempfile::tempdir().expect("repo tempdir");
+    let root = repo.path();
+    run_success(root, &["init"]);
+    run_success(root, &["config", "user.email", "codex@example.com"]);
+    run_success(root, &["config", "user.name", "Codex"]);
+    run_success(root, &["config", "core.autocrlf", "false"]);
+    repo
+}
+
+fn configure_driver(root: &Path, driver: &str, command: &str) {
+    run_success(
+        root,
+        &["config", &format!("merge.{driver}.driver"), command],
+    );
+}
+
+fn configured_marker_exists(root: &Path, marker: &str) -> bool {
+    run(root, &["config", "--get", marker]).0 == 0
+}
+
+fn build_conflicting_patch(root: &Path, attributes: &str) -> String {
+    std::fs::write(root.join(".gitattributes"), attributes).expect("write base attributes");
+    std::fs::write(root.join("target.txt"), "top\nbase\nbottom\n").expect("write base");
+    run_success(root, &["add", ".gitattributes", "target.txt"]);
+    run_success(root, &["commit", "-m", "base"]);
+    let base = run_success(root, &["rev-parse", "HEAD"]);
+    let base = base.trim();
+
+    std::fs::write(root.join("target.txt"), "top\ntheirs\nbottom\n").expect("write theirs");
+    run_success(root, &["add", "target.txt"]);
+    run_success(root, &["commit", "-m", "theirs"]);
+    let patch = run_success(
+        root,
+        &["diff", "--full-index", base, "HEAD", "--", "target.txt"],
+    );
+
+    run_success(root, &["checkout", "-b", "ours", base]);
+    std::fs::write(root.join("target.txt"), "top\nours\nbottom\n").expect("write ours");
+    run_success(root, &["add", "target.txt"]);
+    run_success(root, &["commit", "-m", "ours"]);
+    patch
+}
+
+fn request(root: &Path, diff: String, revert: bool) -> ApplyGitRequest {
+    ApplyGitRequest {
+        cwd: root.to_path_buf(),
+        diff,
+        revert,
+        preflight: false,
+    }
+}
+
+fn index_tree(root: &Path) -> String {
+    run_success(root, &["write-tree"])
+}
+
+fn run_isolated_test(test_name: &str) {
+    let environment = tempfile::tempdir().expect("isolated Git environment");
+    let global_config = environment.path().join("global.gitconfig");
+    let system_config = environment.path().join("system.gitconfig");
+    let trace = environment.path().join("trace.jsonl");
+    std::fs::write(&global_config, "").expect("empty global config");
+    std::fs::write(&system_config, "").expect("empty system config");
+    std::fs::write(&trace, "").expect("empty trace");
+
+    let mut command = std::process::Command::new(std::env::current_exe().expect("test binary"));
+    isolate_git_command_environment(&mut command);
+    let output = command
+        .arg(test_name)
+        .arg("--exact")
+        .arg("--nocapture")
+        .env("CODEX_GIT_UTILS_MERGE_RACE_CHILD", "1")
+        .env("GIT_CONFIG_GLOBAL", &global_config)
+        .env("GIT_CONFIG_SYSTEM", &system_config)
+        .env("GIT_TRACE2_EVENT", &trace)
+        .env("RUST_TEST_THREADS", "1")
+        .output()
+        .expect("run isolated test process");
+    assert!(
+        output.status.success(),
+        "isolated test {test_name} failed:\nstdout:\n{}\nstderr:\n{}",
+        String::from_utf8_lossy(&output.stdout),
+        String::from_utf8_lossy(&output.stderr)
+    );
+}
+
+fn trace_path() -> PathBuf {
+    PathBuf::from(std::env::var_os("GIT_TRACE2_EVENT").expect("trace path"))
+}
+
+fn wait_for_merge_attribute_probe(trace: &Path) -> bool {
+    let deadline = Instant::now() + Duration::from_secs(10);
+    while Instant::now() < deadline {
+        let contents = std::fs::read_to_string(trace).unwrap_or_default();
+        if contents
+            .find("check-attr")
+            .is_some_and(|offset| contents[offset..].contains("\"event\":\"exit\""))
+        {
+            return true;
+        }
+        thread::yield_now();
+    }
+    false
+}
+
+fn append_driver_config(root: &Path, driver: &str, command: &str) {
+    let mut config = OpenOptions::new()
+        .append(true)
+        .open(root.join(".git/config"))
+        .expect("open local config");
+    writeln!(config, "\n[merge \"{driver}\"]\n\tdriver = {command}")
+        .expect("append replacement merge driver");
+}
+
+#[test]
+fn clean_reverse_checks_worktree_before_staging_and_skips_merge_driver() {
+    let repo = init_repo();
+    let root = repo.path();
+    std::fs::write(root.join(".gitattributes"), "file.txt merge=demo\n").expect("write attributes");
+    std::fs::write(root.join("file.txt"), "old\n").expect("write old file");
+    run_success(root, &["add", ".gitattributes", "file.txt"]);
+    run_success(root, &["commit", "-m", "base"]);
+    configure_driver(root, "demo", OLD_DRIVER_COMMAND);
+
+    std::fs::write(root.join("file.txt"), "new\n").expect("write unstaged forward state");
+    let patch = "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-old\n+new\n";
+    let result = apply_git_patch(&request(root, patch.to_string(), /*revert*/ true))
+        .expect("clean reverse apply");
+
+    assert_eq!(result.exit_code, 0, "{}", result.stderr);
+    assert!(result.cmd_for_log.contains("--index"));
+    assert!(!result.cmd_for_log.contains("--3way"));
+    assert_eq!(
+        std::fs::read_to_string(root.join("file.txt")).expect("read restored file"),
+        "old\n"
+    );
+    assert!(!configured_marker_exists(root, "codex.oldmergeran"));
+    assert!(run_success(root, &["status", "--porcelain"]).is_empty());
+}
+
+#[test]
+fn selected_empty_and_equals_named_drivers_reject_without_marker_or_index_mutation() {
+    for (driver, attribute) in [
+        ("", "target.txt merge=\n"),
+        ("x=y", "target.txt merge=x=y\n"),
+    ] {
+        let repo = init_repo();
+        let root = repo.path();
+        let patch = build_conflicting_patch(root, attribute);
+        configure_driver(root, driver, OLD_DRIVER_COMMAND);
+        let before_tree = index_tree(root);
+        let before_contents =
+            std::fs::read_to_string(root.join("target.txt")).expect("read before contents");
+
+        let error = apply_git_patch(&request(root, patch, /*revert*/ false))
+            .expect_err("reject selected merge driver");
+
+        assert_eq!(error.kind(), io::ErrorKind::Unsupported, "{driver:?}");
+        assert!(!configured_marker_exists(root, "codex.oldmergeran"));
+        assert_eq!(index_tree(root), before_tree, "{driver:?}");
+        assert_eq!(
+            std::fs::read_to_string(root.join("target.txt")).expect("read after contents"),
+            before_contents,
+            "{driver:?}"
+        );
+        assert!(run_success(root, &["status", "--porcelain"]).is_empty());
+    }
+}
+
+#[test]
+fn post_probe_attribute_change_cannot_run_empty_named_driver() {
+    if std::env::var_os("CODEX_GIT_UTILS_MERGE_RACE_CHILD").is_none() {
+        run_isolated_test(
+            "merge_driver::race_tests::post_probe_attribute_change_cannot_run_empty_named_driver",
+        );
+        return;
+    }
+
+    let repo = init_repo();
+    let root = repo.path();
+    let patch = build_conflicting_patch(root, "# initially safe\n");
+    configure_driver(root, "", OLD_DRIVER_COMMAND);
+    let trace = trace_path();
+    std::fs::write(&trace, "").expect("clear fixture trace");
+    let attributes = root.join(".gitattributes");
+    let watcher = thread::spawn(move || {
+        let observed = wait_for_merge_attribute_probe(&trace);
+        if observed {
+            std::fs::write(attributes, "target.txt merge=\n")
+                .expect("select empty-name merge driver");
+        }
+        observed
+    });
+
+    let result = apply_git_patch(&request(root, patch, /*revert*/ false))
+        .expect("neutralized three-way apply");
+    assert!(watcher.join().expect("attribute watcher"));
+
+    assert_ne!(result.exit_code, 0);
+    assert!(!configured_marker_exists(root, "codex.oldmergeran"));
+}
+
+#[test]
+fn post_probe_attribute_and_same_driver_command_change_stays_neutralized() {
+    if std::env::var_os("CODEX_GIT_UTILS_MERGE_RACE_CHILD").is_none() {
+        run_isolated_test(
+            "merge_driver::race_tests::post_probe_attribute_and_same_driver_command_change_stays_neutralized",
+        );
+        return;
+    }
+
+    let repo = init_repo();
+    let root = repo.path();
+    let patch = build_conflicting_patch(root, "# initially safe\n");
+    configure_driver(root, "x=y", OLD_DRIVER_COMMAND);
+    let trace = trace_path();
+    std::fs::write(&trace, "").expect("clear fixture trace");
+    let watcher_root = root.to_path_buf();
+    let watcher = thread::spawn(move || {
+        let observed = wait_for_merge_attribute_probe(&trace);
+        if observed {
+            append_driver_config(&watcher_root, "x=y", NEW_DRIVER_COMMAND);
+            std::fs::write(
+                watcher_root.join(".gitattributes"),
+                "target.txt merge=x=y\n",
+            )
+            .expect("select replacement merge driver");
+        }
+        observed
+    });
+
+    let result = apply_git_patch(&request(root, patch, /*revert*/ false))
+        .expect("neutralized three-way apply");
+    assert!(watcher.join().expect("config watcher"));
+
+    assert_ne!(result.exit_code, 0);
+    assert!(!configured_marker_exists(root, "codex.oldmergeran"));
+    assert!(!configured_marker_exists(root, "codex.newmergeran"));
+}
+
+#[test]
+fn same_patch_attribute_activation_does_not_run_merge_driver() {
+    let repo = init_repo();
+    let root = repo.path();
+    std::fs::write(root.join(".gitattributes"), "# initially safe\n")
+        .expect("write base attributes");
+    std::fs::write(root.join("target.txt"), "top\nbase\nbottom\n").expect("write base");
+    run_success(root, &["add", ".gitattributes", "target.txt"]);
+    run_success(root, &["commit", "-m", "base"]);
+    let base = run_success(root, &["rev-parse", "HEAD"]);
+    let base = base.trim();
+
+    std::fs::write(root.join(".gitattributes"), "target.txt merge=x=y\n")
+        .expect("write patch attributes");
+    std::fs::write(root.join("target.txt"), "top\ntheirs\nbottom\n").expect("write theirs");
+    run_success(root, &["add", ".gitattributes", "target.txt"]);
+    run_success(root, &["commit", "-m", "theirs"]);
+    let patch = run_success(root, &["diff", "--full-index", base, "HEAD"]);
+
+    run_success(root, &["checkout", "-b", "ours", base]);
+    std::fs::write(root.join("target.txt"), "top\nours\nbottom\n").expect("write ours");
+    run_success(root, &["add", "target.txt"]);
+    run_success(root, &["commit", "-m", "ours"]);
+    configure_driver(root, "x=y", OLD_DRIVER_COMMAND);
+
+    let result = apply_git_patch(&request(root, patch, /*revert*/ false))
+        .expect("same-patch attribute activation remains neutralized");
+
+    assert_ne!(result.exit_code, 0);
+    assert!(!configured_marker_exists(root, "codex.oldmergeran"));
+}

codex-rs/git-utils/src/safe_git.rs

@@ -4,6 +4,7 @@ use std::io;
 use std::io::Seek;
 use std::io::Write;
 use std::path::Path;
+use std::path::PathBuf;
 use std::process::Command;
 use std::process::Stdio;
 
@@ -15,6 +16,63 @@ use crate::git_config::parse_effective_config_with_origins;
 pub(crate) const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" };
 pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|smudge|process)$";
 
+#[derive(Debug)]
+pub(crate) struct GitConfigOverrideFile {
+    git_config_args: [String; 2],
+    config_path: PathBuf,
+    _config_dir: tempfile::TempDir,
+}
+
+impl GitConfigOverrideFile {
+    pub(crate) fn new(file_name: &str) -> io::Result<Self> {
+        let config_dir = tempfile::tempdir()?;
+        let config_path = config_dir.path().join(file_name);
+        std::fs::write(&config_path, [])?;
+        let config_path_arg = config_path.to_str().ok_or_else(|| {
+            io::Error::new(
+                io::ErrorKind::InvalidData,
+                "non-UTF-8 Git config override path",
+            )
+        })?;
+        Ok(Self {
+            git_config_args: ["-c".to_string(), format!("include.path={config_path_arg}")],
+            config_path,
+            _config_dir: config_dir,
+        })
+    }
+
+    pub(crate) fn git_config_args(&self) -> &[String] {
+        &self.git_config_args
+    }
+
+    pub(crate) fn add_value(
+        &self,
+        git: &GitRunner,
+        cwd: &Path,
+        key: &str,
+        value: &str,
+        description: &str,
+    ) -> io::Result<()> {
+        let mut command = git.command();
+        command
+            .args(["config", "--file"])
+            .arg(&self.config_path)
+            .arg("--add")
+            .arg(key)
+            .arg(value)
+            .current_dir(cwd);
+        let output = git.output(command)?;
+        if !output.status.success() {
+            return Err(io::Error::other(format!(
+                "failed to write {description} (status {}): {}",
+                output.status,
+                String::from_utf8_lossy(&output.stderr).trim()
+            )));
+        }
+        Ok(())
+    }
+}
+
 #[derive(Debug, Clone, PartialEq, Eq)]
 enum FilterAttributeValue {
     Driver(String),
@@ -22,18 +80,15 @@ enum FilterAttributeValue {
 }
 
 pub(crate) struct GitFilterNeutralization {
-    git_config_args: Vec<String>,
-    _config_dir: Option<tempfile::TempDir>,
-    filter_config: BTreeMap<String, GitConfigEntry>,
+    config_override: Option<GitConfigOverrideFile>,
 }
 
 impl GitFilterNeutralization {
     pub(crate) fn git_config_args(&self) -> &[String] {
-        &self.git_config_args
-    }
-
-    pub(crate) fn filter_value(&self, driver: &str, name: &str) -> Option<&str> {
-        effective_filter_value(&self.filter_config, driver, name)
+        self.config_override
+            .as_ref()
+            .map(GitConfigOverrideFile::git_config_args)
+            .unwrap_or_default()
     }
 }
 
@@ -109,9 +164,7 @@ fn ensure_no_selected_executable_git_filters_for(
     };
     if target_drivers.is_empty() {
         return Ok(GitFilterNeutralization {
-            git_config_args: Vec::new(),
-            _config_dir: None,
-            filter_config: entries,
+            config_override: None,
         });
     }
     let paths = paths
@@ -138,9 +191,7 @@ fn ensure_no_selected_executable_git_filters_for(
     }
     if neutralized_drivers.is_empty() {
         return Ok(GitFilterNeutralization {
-            git_config_args: Vec::new(),
-            _config_dir: None,
-            filter_config: entries,
+            config_override: None,
         });
     }
     executable_filter_guard(git, cwd, entries, &neutralized_drivers)
@@ -152,62 +203,33 @@ fn executable_filter_guard(
     filter_config: BTreeMap<String, GitConfigEntry>,
     executable_drivers: &BTreeSet<String>,
 ) -> io::Result<GitFilterNeutralization> {
-    let config_dir = tempfile::tempdir()?;
-    let config_path = config_dir.path().join("filter-neutralization.gitconfig");
-    std::fs::write(&config_path, [])?;
-    let guard = GitFilterNeutralization {
-        git_config_args: vec![
-            "-c".to_string(),
-            format!(
-                "include.path={}",
-                config_path
-                    .to_str()
-                    .ok_or_else(|| invalid_filter_output("non-UTF-8 filter guard path"))?
-            ),
-        ],
-        _config_dir: Some(config_dir),
-        filter_config,
-    };
+    let config_override = GitConfigOverrideFile::new("filter-neutralization.gitconfig")?;
     for driver in executable_drivers {
         debug_assert!(["clean", "smudge", "process"].into_iter().any(|name| {
-            guard
-                .filter_value(driver, name)
+            effective_filter_value(&filter_config, driver, name)
                 .is_some_and(|value| !value.is_empty())
         }));
+        let description = format!("Git filter neutralization for {driver:?}");
         for command in ["clean", "smudge", "process"] {
-            guard.write_config_value(git, cwd, &config_path, driver, command, "")?;
+            config_override.add_value(
+                git,
+                cwd,
+                &format!("filter.{driver}.{command}"),
+                "",
+                &description,
+            )?;
         }
-        guard.write_config_value(git, cwd, &config_path, driver, "required", "false")?;
-    }
-    Ok(guard)
-}
-
-impl GitFilterNeutralization {
-    fn write_config_value(
-        &self,
-        git: &GitRunner,
-        cwd: &Path,
-        config_path: &Path,
-        driver: &str,
-        name: &str,
-        value: &str,
-    ) -> io::Result<()> {
-        let mut command = git.command();
-        command
-            .args(["config", "--file"])
-            .arg(config_path)
-            .args(["--add", &format!("filter.{driver}.{name}"), value])
-            .current_dir(cwd);
-        let output = git.output(command)?;
-        if !output.status.success() {
-            return Err(io::Error::other(format!(
-                "failed to write Git filter neutralization for {driver:?} (status {}): {}",
-                output.status,
-                String::from_utf8_lossy(&output.stderr).trim()
-            )));
-        }
-        Ok(())
+        config_override.add_value(
+            git,
+            cwd,
+            &format!("filter.{driver}.required"),
+            "false",
+            &description,
+        )?;
     }
+    Ok(GitFilterNeutralization {
+        config_override: Some(config_override),
+    })
 }
 
 #[derive(Clone, Copy)]