From 52b21066669e12dfc8ba88d97f92fc325fae16cb Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 12:43:06 -0700 Subject: [PATCH] git-utils: report safe status refusals --- codex-rs/core/src/responses_metadata.rs | 2 + codex-rs/core/src/turn_metadata.rs | 26 +- codex-rs/core/src/turn_metadata_tests.rs | 38 +++ codex-rs/core/tests/responses_headers.rs | 5 +- codex-rs/git-utils/src/errors.rs | 30 ++- codex-rs/git-utils/src/git_command.rs | 21 ++ codex-rs/git-utils/src/info.rs | 115 +++++---- codex-rs/git-utils/src/lib.rs | 2 + codex-rs/git-utils/src/safe_git.rs | 256 +++++++++++++++++++ codex-rs/git-utils/src/safe_git_tests.rs | 300 +++++++++++++++++++++++ 10 files changed, 745 insertions(+), 50 deletions(-) diff --git a/codex-rs/core/src/responses_metadata.rs b/codex-rs/core/src/responses_metadata.rs index 1852c4bea6..c0f4f2cd4c 100644 --- a/codex-rs/core/src/responses_metadata.rs +++ b/codex-rs/core/src/responses_metadata.rs @@ -126,6 +126,8 @@ pub(crate) struct TurnMetadataWorkspace { pub(crate) latest_git_commit_hash: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub(crate) has_changes: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) has_changes_unavailable_reason: Option, } /// Caller-owned snapshot of Codex metadata sent to ResponsesAPI. diff --git a/codex-rs/core/src/turn_metadata.rs b/codex-rs/core/src/turn_metadata.rs index 0a1f781b38..e110aa0b9d 100644 --- a/codex-rs/core/src/turn_metadata.rs +++ b/codex-rs/core/src/turn_metadata.rs @@ -18,8 +18,8 @@ use crate::responses_metadata::subagent_metadata_kind; use crate::sandbox_tags::permission_profile_sandbox_tag; use codex_git_utils::get_git_remote_urls_assume_git_repo; use codex_git_utils::get_git_repo_root; -use codex_git_utils::get_has_changes; use codex_git_utils::get_head_commit_hash; +use codex_git_utils::try_get_has_changes; use codex_protocol::ThreadId; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; @@ -43,6 +43,7 @@ struct WorkspaceGitMetadata { associated_remote_urls: Option>, latest_git_commit_hash: Option, has_changes: Option, + has_changes_unavailable_reason: Option, } impl WorkspaceGitMetadata { @@ -50,6 +51,7 @@ impl WorkspaceGitMetadata { self.associated_remote_urls.is_none() && self.latest_git_commit_hash.is_none() && self.has_changes.is_none() + && self.has_changes_unavailable_reason.is_none() } } @@ -59,6 +61,7 @@ impl From for TurnMetadataWorkspace { associated_remote_urls: value.associated_remote_urls, latest_git_commit_hash: value.latest_git_commit_hash, has_changes: value.has_changes, + has_changes_unavailable_reason: value.has_changes_unavailable_reason, } } } @@ -313,32 +316,36 @@ impl TurnMetadataState { } async fn fetch_workspace_git_metadata(&self) -> WorkspaceGitMetadata { - let (head_commit_hash, associated_remote_urls, has_changes) = tokio::join!( + let (head_commit_hash, associated_remote_urls, has_changes_result) = tokio::join!( get_head_commit_hash(&self.cwd), get_git_remote_urls_assume_git_repo(&self.cwd), - get_has_changes(&self.cwd), + try_get_has_changes(&self.cwd), ); let latest_git_commit_hash = head_commit_hash.map(|sha| sha.0); + let (has_changes, has_changes_unavailable_reason) = split_has_changes(has_changes_result); WorkspaceGitMetadata { associated_remote_urls, latest_git_commit_hash, has_changes, + has_changes_unavailable_reason, } } } async fn memory_workspaces(cwd: &AbsolutePathBuf) -> BTreeMap { let repo_root = get_git_repo_root(cwd).map(|root| root.to_string_lossy().into_owned()); - let (head_commit_hash, associated_remote_urls, has_changes) = tokio::join!( + let (head_commit_hash, associated_remote_urls, has_changes_result) = tokio::join!( get_head_commit_hash(cwd), get_git_remote_urls_assume_git_repo(cwd), - get_has_changes(cwd), + try_get_has_changes(cwd), ); + let (has_changes, has_changes_unavailable_reason) = split_has_changes(has_changes_result); let workspace_git_metadata = WorkspaceGitMetadata { associated_remote_urls, latest_git_commit_hash: head_commit_hash.map(|sha| sha.0), has_changes, + has_changes_unavailable_reason, }; let mut workspaces = BTreeMap::new(); if let Some(repo_root) = repo_root @@ -349,6 +356,15 @@ async fn memory_workspaces(cwd: &AbsolutePathBuf) -> BTreeMap, +) -> (Option, Option) { + match result { + Ok(has_changes) => (Some(has_changes), None), + Err(reason) => (None, Some(reason)), + } +} + #[cfg(test)] #[path = "turn_metadata_tests.rs"] mod tests; diff --git a/codex-rs/core/src/turn_metadata_tests.rs b/codex-rs/core/src/turn_metadata_tests.rs index 83649b9187..be49f1b72c 100644 --- a/codex-rs/core/src/turn_metadata_tests.rs +++ b/codex-rs/core/src/turn_metadata_tests.rs @@ -31,6 +31,44 @@ fn test_mcp_turn_metadata_context() -> McpTurnMetadataContext<'static> { } } +#[test] +fn workspace_metadata_serializes_structured_has_changes_refusal() { + let workspace = TurnMetadataWorkspace { + associated_remote_urls: None, + latest_git_commit_hash: None, + has_changes: None, + has_changes_unavailable_reason: Some( + codex_git_utils::GitReadError::SelectedExecutableFilter { + driver: "lfs".to_string(), + path: "asset.bin".to_string(), + }, + ), + }; + + assert_eq!( + serde_json::to_value(workspace).expect("serialize workspace"), + serde_json::json!({ + "has_changes_unavailable_reason": { + "reason": "selectedExecutableFilter", + "driver": "lfs", + "path": "asset.bin", + } + }) + ); + assert_eq!( + serde_json::to_value(codex_git_utils::GitReadError::CommandFailed { + operation: "status".to_string(), + exit_code: Some(128), + }) + .expect("serialize command failure"), + serde_json::json!({ + "reason": "commandFailed", + "operation": "status", + "exitCode": 128, + }) + ); +} + fn test_responses_metadata_json( state: &TurnMetadataState, window_id: &str, diff --git a/codex-rs/core/tests/responses_headers.rs b/codex-rs/core/tests/responses_headers.rs index 5b7c2c65b8..26b168820f 100644 --- a/codex-rs/core/tests/responses_headers.rs +++ b/codex-rs/core/tests/responses_headers.rs @@ -550,7 +550,10 @@ async fn responses_stream_includes_turn_metadata_header_for_git_workspace_e2e() let request_log = responses::mount_response_sequence( &server, vec![ - responses::sse_response(first_response), + // Git enrichment is intentionally asynchronous. Keep the first + // response open long enough for the follow-up request to observe + // the enriched metadata, even on slower CI hosts. + responses::sse_response(first_response).set_delay(std::time::Duration::from_secs(2)), responses::sse_response(follow_up_response), ], ) diff --git a/codex-rs/git-utils/src/errors.rs b/codex-rs/git-utils/src/errors.rs index 0615a39421..d22c57de25 100644 --- a/codex-rs/git-utils/src/errors.rs +++ b/codex-rs/git-utils/src/errors.rs @@ -2,6 +2,8 @@ use std::path::PathBuf; use std::process::ExitStatus; use std::string::FromUtf8Error; +use serde::Deserialize; +use serde::Serialize; use thiserror::Error; use walkdir::Error as WalkdirError; @@ -34,10 +36,34 @@ pub enum GitToolingError { Io(#[from] std::io::Error), } -#[derive(Clone, Debug, Error, PartialEq, Eq)] -pub(crate) enum GitReadError { +/// Stable, caller-visible reasons that a read-only Git operation was refused +/// or could not produce a complete result. +#[derive(Clone, Debug, Deserialize, Error, PartialEq, Eq, Serialize)] +#[non_exhaustive] +#[serde( + tag = "reason", + rename_all = "camelCase", + rename_all_fields = "camelCase" +)] +pub enum GitReadError { #[error("no trusted Git executable is available")] NoTrustedGit, #[error("{path:?} is not a Git repository")] NotRepository { path: PathBuf }, + #[error("Git resolved {reported_root:?} instead of expected root {expected_root:?}")] + RepositoryRootMismatch { + expected_root: PathBuf, + reported_root: PathBuf, + }, + #[error("Git operation {operation:?} timed out")] + CommandTimedOut { operation: String }, + #[error("Git operation {operation:?} failed with exit code {exit_code:?}")] + CommandFailed { + operation: String, + exit_code: Option, + }, + #[error("Git operation {operation:?} returned invalid output")] + InvalidOutput { operation: String }, + #[error("executable filter {driver:?} is selected for {path:?}")] + SelectedExecutableFilter { driver: String, path: String }, } diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 997578b839..1a83ba82b5 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -7,6 +7,7 @@ use std::process::Command; use crate::errors::GitReadError; use crate::git_config::path_is_within; use crate::safe_git::isolate_git_command_environment; +use crate::safe_git::isolate_tokio_git_command_environment; /// A Git executable outside the repository-controlled roots for one operation. #[derive(Clone, Debug)] @@ -34,12 +35,27 @@ impl GitRunner { Command::new(&self.executable) } + pub(crate) fn tokio_command(&self) -> tokio::process::Command { + tokio::process::Command::new(&self.executable) + } + pub(crate) fn output(&self, mut command: Command) -> io::Result { isolate_git_command_environment(&mut command); command.envs(crate::local_only_git_env()); command.output() } + /// Spawn a configured Tokio command after applying the final + /// repository-selector environment lock. + pub(crate) async fn output_tokio( + &self, + mut command: tokio::process::Command, + ) -> io::Result { + isolate_tokio_git_command_environment(&mut command); + command.envs(crate::local_only_git_env()); + command.output().await + } + fn from_search_path( untrusted: &UntrustedGitLocations, search_path: &OsStr, @@ -73,6 +89,11 @@ impl GitRunner { } Err(GitReadError::NoTrustedGit) } + + #[cfg(all(test, unix))] + pub(crate) fn from_executable_for_test(executable: PathBuf) -> Self { + Self { executable } + } } fn untrusted_git_locations_for_cwd(cwd: &Path) -> Result { diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 28bc561020..2041d510d9 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -13,12 +13,15 @@ use futures::future::join_all; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; -use tokio::process::Command; -use tokio::time::Duration as TokioDuration; use tokio::time::timeout; use ts_rs::TS; +use crate::GitReadError; use crate::GitSha; +use crate::git_command::GitRunner; +use crate::safe_git::DISABLED_HOOKS_PATH; +use crate::safe_git::GIT_COMMAND_TIMEOUT; +use crate::safe_git::selected_executable_filter_from; /// Return `true` if the project folder specified by the `Config` is inside a /// Git repository. @@ -65,10 +68,6 @@ pub async fn get_git_repo_root_with_fs( .ok()? } -/// Timeout for git commands to prevent freezing on large repositories -const GIT_COMMAND_TIMEOUT: TokioDuration = TokioDuration::from_secs(5); -const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; - #[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, TS)] pub struct GitInfo { /// Current commit hash (SHA) @@ -289,15 +288,34 @@ fn trim_git_suffix(value: &str) -> &str { } pub async fn get_has_changes(cwd: &Path) -> Option { - let git = Path::new("git"); - let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; - let output = - run_git_command_with_timeout_from(git, &["status", "--porcelain"], cwd, fsmonitor).await?; + try_get_has_changes(cwd).await.ok() +} + +pub async fn try_get_has_changes(cwd: &Path) -> Result { + let git = GitRunner::for_cwd(cwd)?; + if let Some((driver, path)) = selected_executable_filter_from(&git, cwd).await? { + return Err(GitReadError::SelectedExecutableFilter { + driver, + path: String::from_utf8_lossy(&path).into_owned(), + }); + } + let fsmonitor = detect_local_fsmonitor_override(&git, cwd).await; + let output = try_run_git_command_with_timeout_from( + &git, + &["status", "--porcelain", "--ignore-submodules=dirty"], + cwd, + fsmonitor, + "status", + ) + .await?; if !output.status.success() { - return None; + return Err(GitReadError::CommandFailed { + operation: "status".to_string(), + exit_code: output.status.code(), + }); } - Some(!output.stdout.is_empty()) + Ok(!output.stdout.is_empty()) } fn parse_git_remote_urls(stdout: &str) -> Option> { @@ -405,17 +423,12 @@ pub async fn git_diff_to_remote(cwd: &Path) -> Option { async fn run_git_command_with_timeout(args: &[&str], cwd: &Path) -> Option { // These callers only inspect repository metadata. Worktree workflows probe // once and pass their override directly to the lower-level runner. - run_git_command_with_timeout_from( - Path::new("git"), - args, - cwd, - crate::FsmonitorOverride::Disabled, - ) - .await + let git = GitRunner::for_cwd(cwd).ok()?; + run_git_command_with_timeout_from(&git, args, cwd, crate::FsmonitorOverride::Disabled).await } struct LocalFsmonitorProbeRunner<'a> { - git: &'a Path, + git: &'a GitRunner, cwd: &'a Path, } @@ -423,46 +436,61 @@ impl crate::FsmonitorProbeRunner for LocalFsmonitorProbeRunner<'_> { async fn run_probe(&mut self, args: &[&str]) -> Option> { // Both probes are fast, bounded metadata queries that do not inspect the // worktree or index, so do not reduce the requested command's timeout. - let mut command = Command::new(self.git); - command - .envs(crate::local_only_git_env()) - .args(args) - .current_dir(self.cwd) - .kill_on_drop(true); - match timeout(GIT_COMMAND_TIMEOUT, command.output()).await { + let mut command = self.git.tokio_command(); + command.args(args).current_dir(self.cwd).kill_on_drop(true); + match timeout(GIT_COMMAND_TIMEOUT, self.git.output_tokio(command)).await { Ok(Ok(output)) if output.status.success() => Some(output.stdout), _ => None, } } } -async fn detect_local_fsmonitor_override(git: &Path, cwd: &Path) -> crate::FsmonitorOverride { +async fn detect_local_fsmonitor_override(git: &GitRunner, cwd: &Path) -> crate::FsmonitorOverride { let mut runner = LocalFsmonitorProbeRunner { git, cwd }; crate::detect_fsmonitor_override(&mut runner).await } async fn run_git_command_with_timeout_from( - git: &Path, + git: &GitRunner, args: &[&str], cwd: &Path, fsmonitor: crate::FsmonitorOverride, ) -> Option { - let mut command = Command::new(git); + try_run_git_command_with_timeout_from(git, args, cwd, fsmonitor, "gitMetadata") + .await + .ok() +} + +async fn try_run_git_command_with_timeout_from( + git: &GitRunner, + args: &[&str], + cwd: &Path, + fsmonitor: crate::FsmonitorOverride, + operation: &str, +) -> Result { + let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); + let mut command = git.tokio_command(); command - .envs(crate::local_only_git_env()) .env("GIT_OPTIONAL_LOCKS", "0") // Keep internal Git commands independent of repository-selected hooks - // and fsmonitor helpers while preserving built-in fsmonitor acceleration. - .args(["-c", &format!("core.hooksPath={DISABLED_HOOKS_PATH}")]) + // while preserving built-in fsmonitor acceleration. + .args(["-c", &disabled_hooks]) .args(["-c", fsmonitor.git_config_arg()]) .args(args) .current_dir(cwd) .kill_on_drop(true); - let result = timeout(GIT_COMMAND_TIMEOUT, command.output()).await; - - match result { - Ok(Ok(output)) => Some(output), - _ => None, // Timeout or error + match timeout(GIT_COMMAND_TIMEOUT, git.output_tokio(command)).await { + Err(_) => Err(GitReadError::CommandTimedOut { + operation: operation.to_string(), + }), + Ok(Err(error)) if error.kind() == std::io::ErrorKind::NotFound => { + Err(GitReadError::NoTrustedGit) + } + Ok(Err(_)) => Err(GitReadError::CommandFailed { + operation: operation.to_string(), + exit_code: None, + }), + Ok(Ok(output)) => Ok(output), } } @@ -743,10 +771,10 @@ async fn find_closest_sha(cwd: &Path, branches: &[String], remotes: &[String]) - } async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { - let git = Path::new("git"); - let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; + let git = GitRunner::for_cwd(cwd).ok()?; + let fsmonitor = detect_local_fsmonitor_override(&git, cwd).await; let output = run_git_command_with_timeout_from( - git, + &git, &["diff", "--no-textconv", "--no-ext-diff", &sha.0], cwd, fsmonitor, @@ -761,7 +789,7 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { let mut diff = String::from_utf8(output.stdout).ok()?; if let Some(untracked_output) = run_git_command_with_timeout_from( - git, + &git, &["ls-files", "--others", "--exclude-standard"], cwd, fsmonitor, @@ -779,6 +807,7 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { if !untracked.is_empty() { // Use platform-appropriate null device and guard paths with `--`. let null_device: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; + let git = &git; let futures_iter = untracked.into_iter().map(|file| async move { let file_owned = file; let args_vec: Vec<&str> = vec![ @@ -1089,6 +1118,7 @@ mod tests { // The config response mirrors: // git -c core.fsmonitor=/tmp/fsmonitor-helper \ // config --null --get core.fsmonitor + let git = GitRunner::from_executable_for_test(git); let fsmonitor = detect_local_fsmonitor_override(&git, temp_dir.path()).await; let output = run_git_command_with_timeout_from( &git, @@ -1180,6 +1210,7 @@ mod tests { "write local built-in fsmonitor config" ); + let git = GitRunner::from_executable_for_test(git); let fsmonitor = detect_local_fsmonitor_override(&git, repo.as_path()).await; let output = run_git_command_with_timeout_from( &git, diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index 77918cc841..403192a0d2 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -26,6 +26,7 @@ pub use baseline::ensure_git_baseline_repository; pub use baseline::reset_git_repository; pub use branch::merge_base_with_head; pub use codex_protocol::protocol::GitSha; +pub use errors::GitReadError; pub use errors::GitToolingError; pub use fsmonitor::FsmonitorOverride; pub use fsmonitor::FsmonitorProbeRunner; @@ -47,6 +48,7 @@ pub use info::git_diff_to_remote; pub use info::local_git_branches; pub use info::recent_commits; pub use info::resolve_root_git_project_for_trust; +pub use info::try_get_has_changes; pub use local_only::local_only_git_env; pub use patch_paths::extract_paths_from_patch; pub use patch_paths::stage_paths; diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index 4d28c631c3..1eb46429f4 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -4,9 +4,14 @@ 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; +use tokio::process::Command as TokioCommand; +use tokio::time::Duration; +use tokio::time::timeout; +use crate::GitReadError; use crate::git_command::GitRunner; use crate::git_config::GitConfigEntry; use crate::git_config::parse_effective_config; @@ -14,6 +19,8 @@ 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)$"; +/// Timeout for internal Git commands to prevent freezing on large repositories. +pub(crate) const GIT_COMMAND_TIMEOUT: Duration = Duration::from_secs(5); const ISOLATED_GIT_ENVIRONMENT: [&str; 11] = [ "GIT_DIR", @@ -42,6 +49,86 @@ pub(crate) fn isolate_git_command_environment(command: &mut Command) { } } +pub(crate) fn isolate_tokio_git_command_environment(command: &mut tokio::process::Command) { + for name in ISOLATED_GIT_ENVIRONMENT { + command.env_remove(name); + } +} + +pub(crate) async fn selected_executable_filter_from( + git: &GitRunner, + cwd: &Path, +) -> Result)>, GitReadError> { + let git_root = resolve_git_root_async(git, cwd).await?; + let entries = read_filter_config_async(git, &git_root).await?; + if !entries.values().any(|entry| !entry.value.is_empty()) { + return Ok(None); + } + let paths = read_paths_async(git, &git_root, PathSelection::Tracked).await?; + let attributes = read_filter_attributes_async(git, &git_root, &paths).await?; + selected_executable_filter(&entries, &attributes).map_err(|_| invalid_output("filterSelection")) +} + +async fn resolve_git_root_async(git: &GitRunner, cwd: &Path) -> Result { + let requested_cwd = std::fs::canonicalize(cwd).map_err(|_| GitReadError::NotRepository { + path: cwd.to_path_buf(), + })?; + let expected_root = crate::get_git_repo_root(&requested_cwd) + .and_then(|root| std::fs::canonicalize(root).ok()) + .ok_or_else(|| GitReadError::NotRepository { + path: requested_cwd.clone(), + })?; + let mut command = git.tokio_command(); + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args([ + "-c", + &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c", + "core.fsmonitor=false", + "rev-parse", + "--show-toplevel", + ]) + .current_dir(&requested_cwd) + .kill_on_drop(true); + let output = command_output(git, command, "resolveGitRoot").await?; + if !output.status.success() { + return Err(command_failed("resolveGitRoot", output.status.code())); + } + let reported_root = git_root_from_stdout(output.stdout).and_then(|path| { + std::fs::canonicalize(path).map_err(|_| invalid_output("resolveGitRoot")) + })?; + if reported_root != expected_root { + return Err(GitReadError::RepositoryRootMismatch { + expected_root, + reported_root, + }); + } + Ok(reported_root) +} + +fn git_root_from_stdout(output: Vec) -> Result { + let output = output.strip_suffix(b"\n").unwrap_or(&output); + #[cfg(windows)] + let output = output.strip_suffix(b"\r").unwrap_or(output); + if output.is_empty() { + return Err(invalid_output("resolveGitRoot")); + } + #[cfg(unix)] + { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + Ok(PathBuf::from(OsString::from_vec(output.to_vec()))) + } + #[cfg(not(unix))] + { + String::from_utf8(output.to_vec()) + .map(PathBuf::from) + .map_err(|_| invalid_output("resolveGitRoot")) + } +} + pub(crate) fn ensure_no_selected_executable_git_filters( git: &GitRunner, cwd: &Path, @@ -142,6 +229,158 @@ fn run_effective_config_query( git.output(command) } +async fn read_filter_config_async( + git: &GitRunner, + cwd: &Path, +) -> Result, GitReadError> { + let scoped = run_filter_config_query_async(git, cwd, /*show_scope*/ true).await?; + if scoped + .status + .code() + .is_some_and(|code| code == 0 || code == 1) + { + return parse_effective_config(&scoped.stdout).map_err(|_| invalid_output("filterConfig")); + } + + let legacy = run_filter_config_query_async(git, cwd, /*show_scope*/ false).await?; + if !legacy + .status + .code() + .is_some_and(|code| code == 0 || code == 1) + { + return Err(command_failed("filterConfig", legacy.status.code())); + } + parse_effective_config_with_origins(&legacy.stdout).map_err(|_| invalid_output("filterConfig")) +} + +async fn run_filter_config_query_async( + git: &GitRunner, + cwd: &Path, + show_scope: bool, +) -> Result { + let mut command = git.tokio_command(); + command.args(["config", "--null"]); + if show_scope { + command.arg("--show-scope"); + } + command + .args([ + "--show-origin", + "--includes", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]) + .current_dir(cwd) + .kill_on_drop(true); + command_output(git, command, "filterConfig").await +} + +#[derive(Clone, Copy)] +enum PathSelection { + Tracked, +} + +async fn read_paths_async( + git: &GitRunner, + cwd: &Path, + selection: PathSelection, +) -> Result>, GitReadError> { + let mut command = git.tokio_command(); + let hooks_config = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); + let mut args = vec![ + "-c", + hooks_config.as_str(), + "-c", + "core.fsmonitor=false", + "ls-files", + "-z", + ]; + match selection { + PathSelection::Tracked => args.push("--cached"), + } + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(args) + .current_dir(cwd) + .kill_on_drop(true); + let operation = match selection { + PathSelection::Tracked => "trackedPaths", + }; + let output = command_output(git, command, operation).await?; + if !output.status.success() { + return Err(command_failed(operation, output.status.code())); + } + parse_nul_paths(&output.stdout).map_err(|_| invalid_output(operation)) +} + +async fn read_filter_attributes_async( + git: &GitRunner, + cwd: &Path, + paths: &[Vec], +) -> Result, String>, GitReadError> { + if paths.is_empty() { + return Ok(BTreeMap::new()); + } + let mut input = tempfile::tempfile() + .map_err(|_| command_failed("filterAttributes", /*exit_code*/ None))?; + write_nul_paths(&mut input, paths).map_err(|_| invalid_output("filterAttributes"))?; + input + .rewind() + .map_err(|_| command_failed("filterAttributes", /*exit_code*/ None))?; + + let mut command = git.tokio_command(); + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args([ + "-c", + &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c", + "core.fsmonitor=false", + "check-attr", + "--stdin", + "-z", + "filter", + ]) + .current_dir(cwd) + .stdin(Stdio::from(input)) + .kill_on_drop(true); + let output = command_output(git, command, "filterAttributes").await?; + if !output.status.success() { + return Err(command_failed("filterAttributes", output.status.code())); + } + parse_filter_attributes(&output.stdout, paths).map_err(|_| invalid_output("filterAttributes")) +} + +async fn command_output( + git: &GitRunner, + command: TokioCommand, + operation: &str, +) -> Result { + match timeout(GIT_COMMAND_TIMEOUT, git.output_tokio(command)).await { + Err(_) => Err(GitReadError::CommandTimedOut { + operation: operation.to_string(), + }), + Ok(Err(error)) if error.kind() == io::ErrorKind::NotFound => { + Err(GitReadError::NoTrustedGit) + } + Ok(Err(_)) => Err(command_failed(operation, /*exit_code*/ None)), + Ok(Ok(output)) => Ok(output), + } +} + +fn command_failed(operation: &str, exit_code: Option) -> GitReadError { + GitReadError::CommandFailed { + operation: operation.to_string(), + exit_code, + } +} + +fn invalid_output(operation: &str) -> GitReadError { + GitReadError::InvalidOutput { + operation: operation.to_string(), + } +} + fn read_filter_attributes( git: &GitRunner, cwd: &Path, @@ -213,6 +452,23 @@ fn filter_driver_name(key: &str) -> io::Result { Ok(driver.to_string()) } +fn parse_nul_paths(output: &[u8]) -> io::Result>> { + if output.is_empty() { + return Ok(Vec::new()); + } + let Some(body) = output.strip_suffix(&[0]) else { + return Err(invalid_filter_output("unterminated Git path output")); + }; + let mut paths = Vec::new(); + for path in body.split(|byte| *byte == 0) { + if path.is_empty() { + return Err(invalid_filter_output("empty Git path")); + } + paths.push(path.to_vec()); + } + Ok(paths) +} + fn write_nul_paths(input: &mut std::fs::File, paths: &[Vec]) -> io::Result<()> { let mut unique = BTreeSet::new(); for path in paths { diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs index 5d6efe0147..c39438016e 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -1,9 +1,12 @@ use super::*; +use crate::GitReadError; use crate::apply::ApplyGitRequest; use crate::apply::apply_git_patch; +use crate::get_has_changes; use crate::git_config::GitConfigScope; #[cfg(unix)] use crate::patch_paths::stage_paths; +use crate::try_get_has_changes; use pretty_assertions::assert_eq; use std::collections::BTreeMap; #[cfg(unix)] @@ -198,6 +201,303 @@ async fn ordinary_apply_allows_an_unselected_executable_filter() { ); } +async fn configure_clean_filter(repo_path: &Path, tracked_path: &str) { + std::fs::write( + repo_path.join(".gitattributes"), + format!("{tracked_path} filter=x=y\n"), + ) + .expect("write attributes"); + run_git(repo_path, &["add", ".gitattributes"]).await; + run_git(repo_path, &["commit", "-m", "attributes"]).await; + run_git( + repo_path, + &[ + "config", + "filter.x=y.clean", + "git config codex.filterran true && git hash-object --stdin", + ], + ) + .await; + + let tracked_file = repo_path.join(tracked_path); + let contents = std::fs::read_to_string(&tracked_file).expect("read tracked file"); + std::thread::sleep(std::time::Duration::from_secs(1)); + std::fs::write(tracked_file, contents).expect("refresh tracked file"); +} + +async fn configured_filter_ran(repo_path: &Path) -> bool { + let output = TokioCommand::new("git") + .args(["config", "--get", "codex.filterran"]) + .current_dir(repo_path) + .output() + .await + .expect("read filter marker"); + output.status.success() +} + +async fn add_submodule_with_clean_filter(parent: &Path) { + let source = tempfile::tempdir().expect("submodule source"); + let source_path = source.path(); + run_git(source_path, &["init"]).await; + run_git(source_path, &["config", "user.name", "Test User"]).await; + run_git(source_path, &["config", "user.email", "test@example.com"]).await; + std::fs::write(source_path.join("nested.txt"), "original\n").expect("nested file"); + std::fs::write( + source_path.join(".gitattributes"), + "nested.txt filter=codex-test\n", + ) + .expect("nested attributes"); + run_git(source_path, &["add", "."]).await; + run_git(source_path, &["commit", "-m", "seed"]).await; + + run_git( + parent, + &[ + "-c", + "protocol.file.allow=always", + "submodule", + "add", + source_path.to_str().expect("source path"), + "nested", + ], + ) + .await; + run_git(parent, &["commit", "-m", "add submodule"]).await; + let nested = parent.join("nested"); + run_git( + &nested, + &[ + "config", + "filter.codex-test.clean", + "git config codex.filterran true && git hash-object --stdin", + ], + ) + .await; + std::fs::write(nested.join("nested.txt"), "modified\n").expect("dirty nested file"); +} + +#[tokio::test] +async fn get_has_changes_rejects_configured_clean_filter_without_running_it() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + configure_clean_filter(&repo_path, "test.txt").await; + + assert_eq!(get_has_changes(&repo_path).await, None); + assert_eq!( + try_get_has_changes(&repo_path).await, + Err(GitReadError::SelectedExecutableFilter { + driver: "x=y".to_string(), + path: "test.txt".to_string(), + }) + ); + assert!(!configured_filter_ran(&repo_path).await); +} + +#[tokio::test] +async fn checked_has_changes_distinguishes_non_repository() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + assert_eq!( + try_get_has_changes(temp_dir.path()).await, + Err(GitReadError::NotRepository { + path: std::fs::canonicalize(temp_dir.path()).expect("canonical temp dir"), + }) + ); +} + +#[cfg(target_os = "linux")] +#[tokio::test] +async fn checked_has_changes_accepts_non_utf8_repository_root() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = temp_dir + .path() + .join(OsString::from_vec(b"repo-\xff".to_vec())); + std::fs::create_dir(&repo_path).expect("create non-UTF-8 repository directory"); + run_git(&repo_path, &["init"]).await; + + assert_eq!(try_get_has_changes(&repo_path).await, Ok(false)); +} + +#[cfg(unix)] +#[test] +fn git_root_output_preserves_non_utf8_bytes() { + use std::os::unix::ffi::OsStrExt; + + let path = git_root_from_stdout(b"/tmp/repo-\xff\n".to_vec()).expect("parse raw Git path"); + assert_eq!(path.as_os_str().as_bytes(), b"/tmp/repo-\xff"); +} + +#[cfg(unix)] +#[tokio::test] +async fn root_probe_distinguishes_repository_command_failure() { + use std::os::unix::fs::PermissionsExt; + + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + let failing_git = temp_dir.path().join("failing-git"); + std::fs::write(&failing_git, "#!/bin/sh\nexit 42\n").expect("write failing Git"); + let mut permissions = std::fs::metadata(&failing_git) + .expect("read failing Git metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&failing_git, permissions).expect("mark failing Git executable"); + + let git = GitRunner::from_executable_for_test(failing_git); + assert_eq!( + resolve_git_root_async(&git, &repo_path).await, + Err(GitReadError::CommandFailed { + operation: "resolveGitRoot".to_string(), + exit_code: Some(42), + }) + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn legacy_git_without_show_scope_uses_origin_fallback() { + use std::os::unix::fs::PermissionsExt; + + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + std::fs::write(repo_path.join(".gitattributes"), "test.txt filter=legacy\n") + .expect("attributes"); + run_git(&repo_path, &["add", ".gitattributes"]).await; + run_git(&repo_path, &["commit", "-m", "attributes"]).await; + run_git( + &repo_path, + &["config", "filter.legacy.clean", "git hash-object --stdin"], + ) + .await; + + let output = std::process::Command::new("/bin/sh") + .args(["-c", "command -v git"]) + .output() + .expect("resolve fixture Git"); + assert!(output.status.success(), "resolve fixture Git"); + let real_git = String::from_utf8(output.stdout) + .expect("Git path UTF-8") + .trim() + .to_string(); + let wrapper = temp_dir.path().join("legacy-git"); + std::fs::write( + &wrapper, + format!( + "#!/bin/sh\nfor arg in \"$@\"; do\n if [ \"$arg\" = --show-scope ]; then\n exit 129\n fi\ndone\nexec '{}' \"$@\"\n", + real_git.replace('\'', "'\\''") + ), + ) + .expect("legacy Git wrapper"); + let mut permissions = std::fs::metadata(&wrapper) + .expect("legacy Git metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&wrapper, permissions).expect("legacy Git executable"); + + let git = GitRunner::from_executable_for_test(wrapper); + assert_eq!( + selected_executable_filter_from(&git, &repo_path).await, + Ok(Some(("legacy".to_string(), b"test.txt".to_vec()))) + ); +} + +#[tokio::test] +async fn get_has_changes_rejects_core_worktree_redirection_before_running_filter() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + let redirected = temp_dir.path().join("redirected"); + std::fs::create_dir(&redirected).expect("redirected worktree"); + std::fs::write(redirected.join(".gitattributes"), "test.txt filter=x=y\n").expect("attributes"); + std::fs::write(redirected.join("test.txt"), "redirected content\n").expect("redirected file"); + run_git( + &repo_path, + &[ + "config", + "core.worktree", + redirected.to_str().expect("redirected path"), + ], + ) + .await; + run_git( + &repo_path, + &[ + "config", + "filter.x=y.clean", + "git config codex.filterran true && git hash-object --stdin", + ], + ) + .await; + + assert_eq!(get_has_changes(&repo_path).await, None); + assert!(!configured_filter_ran(&repo_path).await); +} + +#[cfg(unix)] +#[tokio::test] +async fn legacy_git_config_cannot_hide_a_selected_local_filter_from_probe() { + if std::env::var_os("CODEX_GIT_UTILS_SAFE_GIT_ENV_CHILD").is_none() { + use std::os::unix::fs::PermissionsExt; + + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + std::fs::write(repo_path.join(".gitattributes"), "test.txt filter=evil\n") + .expect("attributes"); + run_git(&repo_path, &["add", ".gitattributes"]).await; + run_git(&repo_path, &["commit", "-m", "attributes"]).await; + + let marker = temp_dir.path().join("filter-ran"); + let filter = repo_path.join("clean.sh"); + std::fs::write( + &filter, + format!("#!/bin/sh\n: > '{}'\ncat\n", marker.display()), + ) + .expect("filter script"); + let mut permissions = std::fs::metadata(&filter) + .expect("filter metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&filter, permissions).expect("filter executable"); + run_git( + &repo_path, + &[ + "config", + "--local", + "filter.evil.clean", + filter.to_str().expect("filter path"), + ], + ) + .await; + std::fs::write(repo_path.join("test.txt"), "changed\n").expect("modify tracked file"); + + let safe_config = temp_dir.path().join("safe.gitconfig"); + std::fs::write(&safe_config, "").expect("safe config"); + run_isolated_test( + "safe_git::tests::legacy_git_config_cannot_hide_a_selected_local_filter_from_probe", + &[ + ("CODEX_GIT_UTILS_TARGET_REPO", repo_path.as_os_str()), + ("GIT_CONFIG", safe_config.as_os_str()), + ], + ); + assert!(!marker.exists(), "selected local filter must not run"); + return; + } + + let repo_path = + PathBuf::from(std::env::var_os("CODEX_GIT_UTILS_TARGET_REPO").expect("target repository")); + assert_eq!(get_has_changes(&repo_path).await, None); +} + +#[tokio::test] +async fn get_has_changes_does_not_enter_dirty_submodules() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + add_submodule_with_clean_filter(&repo_path).await; + + assert_eq!(get_has_changes(&repo_path).await, Some(false)); + assert!(!configured_filter_ran(&repo_path.join("nested")).await); +} + #[cfg(unix)] #[tokio::test] async fn apply_and_stage_reject_global_relative_filter_without_running_it() {