agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

[codex] preserve fsmonitor for worktree Git reads (#26880)

Browse Source 
Codex forces `core.fsmonitor=false` on internal Git commands so a
repository cannot select an executable fsmonitor helper. This also
disables Git's built-in daemon for `status`, `diff`, and `ls-files`,
turning those worktree reads into full scans in large repositories.

Read the raw effective `core.fsmonitor` value and preserve it only when
Git interprets it as true and advertises built-in daemon support through
`git version --build-options`. Query uncommon boolean spellings back
through Git using the exact effective value. Unset, false, helper paths,
malformed values, probe failures, and unsupported Git builds continue to
force `core.fsmonitor=false`.

Centralize this policy in `git-utils` while keeping process execution in
the existing local and workspace-command adapters. Probe once per
worktree workflow and reuse the result for its Git commands, including
the TUI `/diff` path. Metadata-only commands and repository discovery
remain disabled without probing. Each probe and requested Git process
keeps its own existing timeout, and the decision is not cached because
layered and conditional Git configuration can change while Codex runs.

---------

Co-authored-by: Chris Bookholt <bookholt@openai.com>
This commit is contained in:
Tamir Duberstein
2026-06-08 21:32:46 -07:00
committed by GitHub
Unverified
parent 08cb633c06
commit dffc4bf75d
6 changed files with 831 additions and 203 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/get_git_diff.rs
+350 -153
View File
@@ -11,9 +11,11 @@ use std::time::Duration;
use crate::workspace_command::WorkspaceCommand;
use crate::workspace_command::WorkspaceCommandExecutor;
use crate::workspace_command::WorkspaceCommandOutput;
use codex_git_utils::FsmonitorOverride;
use codex_git_utils::FsmonitorProbeRunner;
use codex_git_utils::detect_fsmonitor_override;
const DIFF_COMMAND_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 30);
const DISABLE_FSMONITOR_CONFIG: &str = "core.fsmonitor=false";
const DISABLE_HOOKS_CONFIG: &str = if cfg!(windows) {
"core.hooksPath=NUL"
} else {
@@ -21,6 +23,25 @@ const DISABLE_HOOKS_CONFIG: &str = if cfg!(windows) {
};
const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|process)$";
// `/diff` may execute Git through a remote workspace, so git-utils owns the
// probe policy while this adapter keeps command execution in the TUI layer.
// WorkspaceCommand bounds each call; `/diff` has no aggregate command deadline.
struct WorkspaceFsmonitorProbeRunner<'a> {
runner: &'a dyn WorkspaceCommandExecutor,
cwd: &'a Path,
}
impl FsmonitorProbeRunner for WorkspaceFsmonitorProbeRunner<'_> {
async fn run_probe(&mut self, args: &[&str]) -> Option<Vec<u8>> {
let argv = ["git"].into_iter().chain(args.iter().copied());
let command = WorkspaceCommand::new(argv).cwd(self.cwd.to_path_buf());
match self.runner.run(command).await {
Ok(output) if output.success() => Some(output.stdout.into_bytes()),
_ => None,
}
}
}
/// Return value of [`get_git_diff`].
///
/// * `bool` – Whether the current working directory is inside a Git repo.
@@ -34,12 +55,17 @@ pub(crate) async fn get_git_diff(
return Ok((false, String::new()));
}
// Probe once per `/diff` and reuse the result for all subsequent Git commands.
let mut probe_runner = WorkspaceFsmonitorProbeRunner { runner, cwd };
let fsmonitor = detect_fsmonitor_override(&mut probe_runner).await;
// Keep `/diff` informational: repository configuration must not select executable diff helpers.
let diff_config_overrides = diff_filter_config_overrides(runner, cwd).await?;
let diff_config_overrides = diff_filter_config_overrides(runner, cwd, fsmonitor).await?;
let (tracked_diff_res, untracked_output_res) = tokio::join!(
run_git_capture_diff(
runner,
cwd,
fsmonitor,
&diff_config_overrides,
&[
"diff",
@@ -50,7 +76,12 @@ pub(crate) async fn get_git_diff(
"--color",
]
),
run_git_capture_stdout(runner, cwd, &["ls-files", "--others", "--exclude-standard"]),
run_git_capture_stdout(
runner,
cwd,
fsmonitor,
&["ls-files", "--others", "--exclude-standard"]
),
);
let tracked_diff = tracked_diff_res?;
let untracked_output = untracked_output_res?;
@@ -80,7 +111,8 @@ pub(crate) async fn get_git_diff(
null_path,
file,
];
let diff = run_git_capture_diff(runner, cwd, &diff_config_overrides, &args).await?;
let diff =
run_git_capture_diff(runner, cwd, fsmonitor, &diff_config_overrides, &args).await?;
untracked_diff.push_str(&diff);
}
@@ -92,9 +124,10 @@ pub(crate) async fn get_git_diff(
async fn run_git_capture_stdout(
runner: &dyn WorkspaceCommandExecutor,
cwd: &Path,
fsmonitor: FsmonitorOverride,
args: &[&str],
) -> Result<String, String> {
let output = run_git_command(runner, cwd, &[], args).await?;
let output = run_git_command(runner, cwd, fsmonitor, &[], args).await?;
if output.success() {
Ok(output.stdout)
} else {
@@ -110,10 +143,11 @@ async fn run_git_capture_stdout(
async fn run_git_capture_diff(
runner: &dyn WorkspaceCommandExecutor,
cwd: &Path,
fsmonitor: FsmonitorOverride,
config_overrides: &[(String, String)],
args: &[&str],
) -> Result<String, String> {
let output = run_git_command(runner, cwd, config_overrides, args).await?;
let output = run_git_command(runner, cwd, fsmonitor, config_overrides, args).await?;
if output.success() || output.exit_code == 1 {
Ok(output.stdout)
} else {
@@ -129,6 +163,7 @@ async fn run_git_capture_diff(
async fn diff_filter_config_overrides(
runner: &dyn WorkspaceCommandExecutor,
cwd: &Path,
fsmonitor: FsmonitorOverride,
) -> Result<Vec<(String, String)>, String> {
let args = [
"config",
@@ -137,7 +172,7 @@ async fn diff_filter_config_overrides(
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
];
let output = run_git_command(runner, cwd, &[], &args).await?;
let output = run_git_command(runner, cwd, fsmonitor, &[], &args).await?;
if output.exit_code != 0 && output.exit_code != 1 {
return Err(format!(
"git {:?} failed with status {}",
@@ -174,25 +209,35 @@ async fn inside_git_repo(
runner: &dyn WorkspaceCommandExecutor,
cwd: &Path,
) -> Result<bool, String> {
let output = run_git_command(runner, cwd, &[], &["rev-parse", "--is-inside-work-tree"]).await?;
// `rev-parse` does not inspect the worktree, and probing before this check
// would also run extra Git commands outside repositories.
let output = run_git_command(
runner,
cwd,
FsmonitorOverride::Disabled,
&[],
&["rev-parse", "--is-inside-work-tree"],
)
.await?;
Ok(output.success())
}
async fn run_git_command(
runner: &dyn WorkspaceCommandExecutor,
cwd: &Path,
fsmonitor: FsmonitorOverride,
config_overrides: &[(String, String)],
args: &[&str],
) -> Result<WorkspaceCommandOutput, String> {
let mut argv = Vec::with_capacity(args.len() + 5);
argv.push("git".to_string());
argv.extend([
"-c".to_string(),
DISABLE_FSMONITOR_CONFIG.to_string(),
"-c".to_string(),
DISABLE_HOOKS_CONFIG.to_string(),
]);
argv.extend(args.iter().map(|arg| (*arg).to_string()));
let argv = [
"git",
"-c",
fsmonitor.git_config_arg(),
"-c",
DISABLE_HOOKS_CONFIG,
]
.into_iter()
.chain(args.iter().copied());
let mut command = WorkspaceCommand::new(argv)
.cwd(cwd.to_path_buf())
.timeout(DIFF_COMMAND_TIMEOUT)
@@ -230,7 +275,10 @@ mod tests {
async fn get_git_diff_returns_not_git_for_non_git_cwd() {
let cwd = PathBuf::from("/workspace");
let runner = FakeRunner::new(vec![response(
git_command(&["rev-parse", "--is-inside-work-tree"]),
git_command(
FsmonitorOverride::Disabled,
&["rev-parse", "--is-inside-work-tree"],
),
/*exit_code*/ 128,
"",
)]);
@@ -238,11 +286,7 @@ mod tests {
let result = get_git_diff(&runner, &cwd).await;
assert_eq!(result, Ok((false, String::new())));
assert_commands(
&runner.commands(),
&[git_command(&["rev-parse", "--is-inside-work-tree"])],
&cwd,
);
assert_command_metadata(&runner.commands(), &cwd);
}
#[tokio::test]
@@ -250,51 +294,84 @@ mod tests {
let cwd = PathBuf::from("/workspace");
let runner = FakeRunner::new(vec![
response(
git_command(&["rev-parse", "--is-inside-work-tree"]),
git_command(
FsmonitorOverride::Disabled,
&["rev-parse", "--is-inside-work-tree"],
),
/*exit_code*/ 0,
"true\n",
),
response(
git_command(&[
git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]),
/*exit_code*/ 0,
"/tmp/fsmonitor-helper\0",
),
response(
git_probe_command(&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
"--type=bool",
"--fixed-value",
"--get",
"core.fsmonitor",
"/tmp/fsmonitor-helper",
]),
/*exit_code*/ 128,
"",
),
response(
git_command(
FsmonitorOverride::Disabled,
&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
],
),
/*exit_code*/ 0,
"filter.evil.clean\0filter.evil.process\0",
),
response(
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
]),
git_command(
FsmonitorOverride::Disabled,
&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
],
),
/*exit_code*/ 1,
"tracked\n",
),
response(
git_command(&["ls-files", "--others", "--exclude-standard"]),
git_command(
FsmonitorOverride::Disabled,
&["ls-files", "--others", "--exclude-standard"],
),
/*exit_code*/ 0,
"new.txt\n",
),
response(
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
"--no-index",
"--",
null_device(),
"new.txt",
]),
git_command(
FsmonitorOverride::Disabled,
&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
"--no-index",
"--",
null_device(),
"new.txt",
],
),
/*exit_code*/ 1,
"untracked\n",
),
@@ -304,43 +381,95 @@ mod tests {
assert_eq!(result, Ok((true, "tracked\nuntracked\n".to_string())));
let commands = runner.commands();
assert_commands(
&commands,
&[
git_command(&["rev-parse", "--is-inside-work-tree"]),
git_command(&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
]),
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
]),
git_command(&["ls-files", "--others", "--exclude-standard"]),
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
"--no-index",
"--",
null_device(),
"new.txt",
]),
],
&cwd,
);
assert_eq!(commands[2].env, filter_override_env("filter.evil"));
assert_command_metadata(&commands, &cwd);
assert_eq!(commands[4].env, filter_override_env("filter.evil"));
assert_eq!(commands[6].env, filter_override_env("filter.evil"));
}
#[tokio::test]
async fn get_git_diff_preserves_builtin_fsmonitor_for_diff_workflow() {
let cwd = PathBuf::from("/workspace");
let runner = FakeRunner::new(vec![
response(
git_command(
FsmonitorOverride::Disabled,
&["rev-parse", "--is-inside-work-tree"],
),
/*exit_code*/ 0,
"true\n",
),
response(
git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]),
/*exit_code*/ 0,
"true\0",
),
response(
git_probe_command(&["version", "--build-options"]),
/*exit_code*/ 0,
"feature: fsmonitor--daemon\n",
),
response(
git_command(
FsmonitorOverride::BuiltIn,
&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
],
),
/*exit_code*/ 1,
"",
),
response(
git_command(
FsmonitorOverride::BuiltIn,
&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
],
),
/*exit_code*/ 1,
"tracked\n",
),
response(
git_command(
FsmonitorOverride::BuiltIn,
&["ls-files", "--others", "--exclude-standard"],
),
/*exit_code*/ 0,
"new.txt\n",
),
response(
git_command(
FsmonitorOverride::BuiltIn,
&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
"--no-index",
"--",
null_device(),
"new.txt",
],
),
/*exit_code*/ 1,
"untracked\n",
),
]);
let result = get_git_diff(&runner, &cwd).await;
assert_eq!(result, Ok((true, "tracked\nuntracked\n".to_string())));
assert_command_metadata(&runner.commands(), &cwd);
}
#[tokio::test]
@@ -348,35 +477,52 @@ mod tests {
let cwd = PathBuf::from("/workspace");
let runner = FakeRunner::new(vec![
response(
git_command(&["rev-parse", "--is-inside-work-tree"]),
git_command(
FsmonitorOverride::Disabled,
&["rev-parse", "--is-inside-work-tree"],
),
/*exit_code*/ 0,
"true\n",
),
response(
git_command(&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
]),
git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]),
/*exit_code*/ 1,
"",
),
response(
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
]),
git_command(
FsmonitorOverride::Disabled,
&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
],
),
/*exit_code*/ 1,
"",
),
response(
git_command(
FsmonitorOverride::Disabled,
&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
],
),
/*exit_code*/ 1,
"tracked\n",
),
response(
git_command(&["ls-files", "--others", "--exclude-standard"]),
git_command(
FsmonitorOverride::Disabled,
&["ls-files", "--others", "--exclude-standard"],
),
/*exit_code*/ 0,
"",
),
@@ -385,6 +531,7 @@ mod tests {
let result = get_git_diff(&runner, &cwd).await;
assert_eq!(result, Ok((true, "tracked\n".to_string())));
assert_command_metadata(&runner.commands(), &cwd);
}
#[tokio::test]
@@ -392,35 +539,52 @@ mod tests {
let cwd = PathBuf::from("/workspace");
let runner = FakeRunner::new(vec![
response(
git_command(&["rev-parse", "--is-inside-work-tree"]),
git_command(
FsmonitorOverride::Disabled,
&["rev-parse", "--is-inside-work-tree"],
),
/*exit_code*/ 0,
"true\n",
),
response(
git_command(&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
]),
git_probe_command(&["config", "--null", "--get", "core.fsmonitor"]),
/*exit_code*/ 1,
"",
),
response(
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
]),
git_command(
FsmonitorOverride::Disabled,
&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
],
),
/*exit_code*/ 1,
"",
),
response(
git_command(
FsmonitorOverride::Disabled,
&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
],
),
/*exit_code*/ 2,
"",
),
response(
git_command(&["ls-files", "--others", "--exclude-standard"]),
git_command(
FsmonitorOverride::Disabled,
&["ls-files", "--others", "--exclude-standard"],
),
/*exit_code*/ 0,
"",
),
@@ -430,12 +594,11 @@ mod tests {
.await
.expect_err("unexpected git diff status should fail");
assert!(
error.contains(
"git [\"diff\", \"--no-textconv\", \"--no-ext-diff\", \"--submodule=short\", \"--ignore-submodules=dirty\", \"--color\"] failed with status 2"
),
"unexpected error: {error}",
assert_eq!(
error,
"git [\"diff\", \"--no-textconv\", \"--no-ext-diff\", \"--submodule=short\", \"--ignore-submodules=dirty\", \"--color\"] failed with status 2"
);
assert_command_metadata(&runner.commands(), &cwd);
}
#[cfg(unix)]
@@ -505,11 +668,18 @@ mod tests {
.await
.expect("generate diff without invoking helpers");
assert!(result.1.contains("before"));
assert!(result.1.contains("after"));
assert!(!filter_helper.with_extension("sh.ran").exists());
assert!(!fsmonitor_helper.with_extension("sh.ran").exists());
assert!(!hook_helper.with_extension("sh.ran").exists());
assert_eq!(
(
result.1.contains("before"),
result.1.contains("after"),
filter_helper.with_extension("sh.ran").exists(),
fsmonitor_helper.with_extension("sh.ran").exists(),
hook_helper.with_extension("sh.ran").exists(),
),
(true, true, false, false, false),
"diff:\n{}",
result.1
);
}
#[cfg(unix)]
@@ -565,20 +735,32 @@ mod tests {
.await
.expect("generate diff without inspecting submodule worktrees");
assert!(result.1.is_empty());
assert!(!helper.with_extension("sh.ran").exists());
assert_eq!(
(result.1, helper.with_extension("sh.ran").exists()),
(String::new(), false)
);
}
fn git_command(args: &[&str]) -> Vec<String> {
let mut argv = vec![
"git".to_string(),
"-c".to_string(),
DISABLE_FSMONITOR_CONFIG.to_string(),
"-c".to_string(),
DISABLE_HOOKS_CONFIG.to_string(),
];
argv.extend(args.iter().map(|arg| (*arg).to_string()));
argv
fn git_command(fsmonitor: FsmonitorOverride, args: &[&str]) -> Vec<String> {
[
"git",
"-c",
fsmonitor.git_config_arg(),
"-c",
DISABLE_HOOKS_CONFIG,
]
.into_iter()
.chain(args.iter().copied())
.map(str::to_string)
.collect()
}
fn git_probe_command(args: &[&str]) -> Vec<String> {
["git"]
.into_iter()
.chain(args.iter().copied())
.map(str::to_string)
.collect()
}
fn filter_override_env(driver: &str) -> HashMap<String, Option<String>> {
@@ -619,12 +801,18 @@ mod tests {
#[cfg(unix)]
fn run_git_setup(cwd: &Path, args: &[&str]) {
let status = ProcessCommand::new("git")
let output = ProcessCommand::new("git")
.args(args)
.current_dir(cwd)
.status()
.output()
.expect("run git setup command");
assert!(status.success(), "git setup command failed: {args:?}");
assert_eq!(
output.status.code(),
Some(0),
"git setup command failed: {args:?}\nstdout:\n{}\nstderr:\n{}",
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr)
);
}
#[cfg(unix)]
@@ -638,17 +826,21 @@ mod tests {
fs::set_permissions(path, permissions).expect("make helper executable");
}
fn assert_commands(commands: &[WorkspaceCommand], expected: &[Vec<String>], cwd: &Path) {
let actual: Vec<Vec<String>> = commands
.iter()
.map(|command| command.argv.clone())
.collect();
assert_eq!(actual, expected);
fn assert_command_metadata(commands: &[WorkspaceCommand], cwd: &Path) {
for command in commands {
assert_eq!(command.cwd.as_deref(), Some(cwd));
assert_eq!(command.timeout, DIFF_COMMAND_TIMEOUT);
assert!(command.disable_output_cap);
if matches!(
command.argv.get(1).map(String::as_str),
Some("config" | "version")
) {
assert_eq!(command.env, HashMap::new());
assert_eq!(command.timeout, Duration::from_secs(/*secs*/ 5));
assert_eq!(command.output_bytes_cap, 64 * 1024);
assert_eq!(command.disable_output_cap, false);
} else {
assert_eq!(command.timeout, DIFF_COMMAND_TIMEOUT);
assert_eq!(command.disable_output_cap, true);
}
}
}
@@ -671,6 +863,11 @@ mod tests {
}
fn commands(&self) -> Vec<WorkspaceCommand> {
assert_eq!(
self.responses.lock().expect("responses lock").len(),
0,
"unused fake responses"
);
self.commands.lock().expect("commands lock").clone()
}
}