From 75f2ff379c648b4d5ca6f904f584311ed396a9b3 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 02:52:07 -0700 Subject: [PATCH 1/8] test(core): surface review lifecycle failures --- codex-rs/core/tests/suite/review.rs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index af44baf6ca..f6e177c2e6 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -26,6 +26,7 @@ use core_test_support::skip_if_no_network; use core_test_support::test_codex::local_selections; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; +use core_test_support::wait_for_event_match; use pretty_assertions::assert_eq; use std::path::PathBuf; use std::sync::Arc; @@ -843,8 +844,30 @@ async fn review_uses_overridden_cwd_for_base_branch_merge_base() { .await .unwrap(); - let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; - let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + wait_for_event_match(&codex, |event| match event { + EventMsg::EnteredReviewMode(_) => Some(Ok(())), + EventMsg::Error(error) => Some(Err(format!( + "review failed before entering review mode: {error:#?}" + ))), + EventMsg::TurnAborted(aborted) => Some(Err(format!( + "review aborted before entering review mode: {aborted:#?}" + ))), + _ => None, + }) + .await + .unwrap_or_else(|error| panic!("{error}")); + wait_for_event_match(&codex, |event| match event { + EventMsg::TurnComplete(_) => Some(Ok(())), + EventMsg::Error(error) => Some(Err(format!( + "review failed before completing the turn: {error:#?}" + ))), + EventMsg::TurnAborted(aborted) => Some(Err(format!( + "review aborted before completing the turn: {aborted:#?}" + ))), + _ => None, + }) + .await + .unwrap_or_else(|error| panic!("{error}")); let requests = request_log.requests(); assert_eq!(requests.len(), 1); From fcd31e89a25ac3651e0d49b0d436f74453721f84 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 03:43:53 -0700 Subject: [PATCH 2/8] core: order review lifecycle entry before completion --- codex-rs/core/src/session/review.rs | 11 +++---- codex-rs/core/src/session/tests.rs | 32 ++++++++++++++++-- codex-rs/core/src/tasks/review.rs | 50 ++++++++++++++++++++++++++--- codex-rs/core/tests/suite/review.rs | 6 ++++ 4 files changed, 85 insertions(+), 14 deletions(-) diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index 1587290d33..2d734db2b0 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -159,16 +159,13 @@ pub(super) async fn spawn_review_thread( if tc.environments.single_local_environment_cwd().is_some() { tc.turn_metadata_state.spawn_git_enrichment_task(); } - // TODO(ccunningham): Review turns currently rely on `spawn_task` for TurnComplete but do not - // emit a parent TurnStarted. Consider giving review a full parent turn lifecycle - // (TurnStarted + TurnComplete) for consistency with other standalone tasks. - sess.spawn_task(tc.clone(), input, ReviewTask::new()).await; - - // Announce entering review mode so UIs can switch modes. let review_request = ReviewRequest { target: resolved.target, user_facing_hint: Some(resolved.user_facing_hint), }; - sess.send_event(&tc, EventMsg::EnteredReviewMode(review_request)) + // TODO(ccunningham): Review turns currently rely on `spawn_task` for TurnComplete but do not + // emit a parent TurnStarted. Consider giving review a full parent turn lifecycle + // (TurnStarted + TurnComplete) for consistency with other standalone tasks. + sess.spawn_task(tc, input, ReviewTask::new(review_request)) .await; } diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index ece9d446b5..f46b0842a4 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -131,6 +131,8 @@ use codex_protocol::protocol::RealtimeConversationListVoicesResponseEvent; use codex_protocol::protocol::RealtimeVoice; use codex_protocol::protocol::RealtimeVoicesList; use codex_protocol::protocol::ResumedHistory; +use codex_protocol::protocol::ReviewRequest; +use codex_protocol::protocol::ReviewTarget; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::SessionMeta; use codex_protocol::protocol::SessionMetaLine; @@ -10386,14 +10388,24 @@ async fn abort_review_task_emits_exited_then_aborted_and_records_history() { }], client_id: None, }]; - sess.spawn_task(Arc::clone(&tc), input, ReviewTask::new()) - .await; + sess.spawn_task( + Arc::clone(&tc), + input, + ReviewTask::new(ReviewRequest { + target: ReviewTarget::Custom { + instructions: "review".to_string(), + }, + user_facing_hint: None, + }), + ) + .await; sess.abort_all_tasks(TurnAbortReason::Interrupted).await; - // Aborting a review task should exit review mode before surfacing the abort to the client. + // Aborting a review task should enter and exit review mode before surfacing the abort. // We scan for these events (rather than relying on fixed ordering) since unrelated events // may interleave. + let mut entered_review_mode_idx = None; let mut exited_review_mode_idx = None; let mut turn_aborted_idx = None; let mut idx = 0usize; @@ -10407,6 +10419,12 @@ async fn abort_review_task_emits_exited_then_aborted_and_records_history() { let event_idx = idx; idx = idx.saturating_add(1); match evt.msg { + EventMsg::EnteredReviewMode(_) => { + assert!( + entered_review_mode_idx.replace(event_idx).is_none(), + "expected exactly one EnteredReviewMode" + ); + } EventMsg::ExitedReviewMode(ev) => { assert!(ev.review_output.is_none()); exited_review_mode_idx = Some(event_idx); @@ -10419,6 +10437,10 @@ async fn abort_review_task_emits_exited_then_aborted_and_records_history() { _ => {} } } + assert!( + entered_review_mode_idx.is_some(), + "expected EnteredReviewMode before abort" + ); assert!( exited_review_mode_idx.is_some(), "expected ExitedReviewMode after abort" @@ -10427,6 +10449,10 @@ async fn abort_review_task_emits_exited_then_aborted_and_records_history() { turn_aborted_idx.is_some(), "expected TurnAborted after abort" ); + assert!( + entered_review_mode_idx.unwrap() < exited_review_mode_idx.unwrap(), + "expected EnteredReviewMode before ExitedReviewMode" + ); assert!( exited_review_mode_idx.unwrap() < turn_aborted_idx.unwrap(), "expected ExitedReviewMode before TurnAborted" diff --git a/codex-rs/core/src/tasks/review.rs b/codex-rs/core/src/tasks/review.rs index 19ac0fdc35..3c0a14d443 100644 --- a/codex-rs/core/src/tasks/review.rs +++ b/codex-rs/core/src/tasks/review.rs @@ -13,7 +13,12 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExitedReviewModeEvent; use codex_protocol::protocol::ItemCompletedEvent; use codex_protocol::protocol::ReviewOutputEvent; +use codex_protocol::protocol::ReviewRequest; use codex_protocol::protocol::SubAgentSource; +use futures::FutureExt; +use futures::future::BoxFuture; +use futures::future::Shared; +use tokio::sync::OnceCell; use tokio_util::sync::CancellationToken; use crate::codex_delegate::run_codex_thread_one_shot; @@ -31,12 +36,47 @@ use super::SessionTask; use super::SessionTaskContext; use super::SessionTaskResult; -#[derive(Clone, Copy)] -pub(crate) struct ReviewTask; +type EnterReviewModeFuture = Shared>; + +pub(crate) struct ReviewTask { + review_request: ReviewRequest, + enter_review_mode: OnceCell, +} impl ReviewTask { - pub(crate) fn new() -> Self { - Self + pub(crate) fn new(review_request: ReviewRequest) -> Self { + Self { + review_request, + enter_review_mode: OnceCell::new(), + } + } + + async fn enter_review_mode_once( + &self, + session: &Arc, + ctx: &Arc, + ) { + // Normal completion and abort can race. Both paths await the same shared send future so + // neither ExitedReviewMode nor a terminal turn event can overtake EnteredReviewMode. + let enter_review_mode = self + .enter_review_mode + .get_or_init(|| { + let session = session.clone_session(); + let ctx = Arc::clone(ctx); + let review_request = self.review_request.clone(); + std::future::ready( + async move { + session + .send_event(ctx.as_ref(), EventMsg::EnteredReviewMode(review_request)) + .await; + } + .boxed() + .shared(), + ) + }) + .await + .clone(); + enter_review_mode.await; } } @@ -56,6 +96,7 @@ impl SessionTask for ReviewTask { input: Vec, cancellation_token: CancellationToken, ) -> SessionTaskResult { + self.enter_review_mode_once(&session, &ctx).await; session.session.services.session_telemetry.counter( "codex.task.review", /*inc*/ 1, @@ -89,6 +130,7 @@ impl SessionTask for ReviewTask { } async fn abort(&self, session: Arc, ctx: Arc) { + self.enter_review_mode_once(&session, &ctx).await; exit_review_mode(session.clone_session(), /*review_output*/ None, ctx).await; } } diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index f6e177c2e6..31e7e17f82 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -852,6 +852,12 @@ async fn review_uses_overridden_cwd_for_base_branch_merge_base() { EventMsg::TurnAborted(aborted) => Some(Err(format!( "review aborted before entering review mode: {aborted:#?}" ))), + EventMsg::ExitedReviewMode(exited) => Some(Err(format!( + "review exited before entering review mode: {exited:#?}" + ))), + EventMsg::TurnComplete(completed) => Some(Err(format!( + "review completed before entering review mode: {completed:#?}" + ))), _ => None, }) .await From 7c0a4f4363ec4e30fc1769f1b08efd29a4fcb871 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 04:07:40 -0700 Subject: [PATCH 3/8] test(core): distinguish review timeout phases --- codex-rs/core/tests/suite/review.rs | 89 ++++++++++++++++++++--------- 1 file changed, 62 insertions(+), 27 deletions(-) diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 31e7e17f82..956b5a4b54 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -26,12 +26,12 @@ use core_test_support::skip_if_no_network; use core_test_support::test_codex::local_selections; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; -use core_test_support::wait_for_event_match; use pretty_assertions::assert_eq; use std::path::PathBuf; use std::sync::Arc; use tempfile::TempDir; use tokio::io::AsyncWriteExt as _; +use tokio::time::Duration; use uuid::Uuid; use wiremock::MockServer; @@ -844,36 +844,71 @@ async fn review_uses_overridden_cwd_for_base_branch_merge_base() { .await .unwrap(); - wait_for_event_match(&codex, |event| match event { - EventMsg::EnteredReviewMode(_) => Some(Ok(())), - EventMsg::Error(error) => Some(Err(format!( - "review failed before entering review mode: {error:#?}" - ))), - EventMsg::TurnAborted(aborted) => Some(Err(format!( - "review aborted before entering review mode: {aborted:#?}" - ))), - EventMsg::ExitedReviewMode(exited) => Some(Err(format!( - "review exited before entering review mode: {exited:#?}" - ))), - EventMsg::TurnComplete(completed) => Some(Err(format!( - "review completed before entering review mode: {completed:#?}" - ))), - _ => None, + let entered_review_mode = tokio::time::timeout(Duration::from_secs(10), async { + loop { + let event = codex.next_event().await.expect("stream ended unexpectedly"); + match event.msg { + EventMsg::EnteredReviewMode(_) => return Ok(()), + EventMsg::Error(error) => { + return Err(format!( + "review failed before entering review mode: {error:#?}" + )); + } + EventMsg::TurnAborted(aborted) => { + return Err(format!( + "review aborted before entering review mode: {aborted:#?}" + )); + } + EventMsg::ExitedReviewMode(exited) => { + return Err(format!( + "review exited before entering review mode: {exited:#?}" + )); + } + EventMsg::TurnComplete(completed) => { + return Err(format!( + "review completed before entering review mode: {completed:#?}" + )); + } + _ => {} + } + } }) .await - .unwrap_or_else(|error| panic!("{error}")); - wait_for_event_match(&codex, |event| match event { - EventMsg::TurnComplete(_) => Some(Ok(())), - EventMsg::Error(error) => Some(Err(format!( - "review failed before completing the turn: {error:#?}" - ))), - EventMsg::TurnAborted(aborted) => Some(Err(format!( - "review aborted before completing the turn: {aborted:#?}" - ))), - _ => None, + .unwrap_or_else(|_| { + panic!( + "timeout before entering review mode; mock response requests observed: {}", + request_log.requests().len() + ) + }); + entered_review_mode.unwrap_or_else(|error| panic!("{error}")); + + let completed_turn = tokio::time::timeout(Duration::from_secs(10), async { + loop { + let event = codex.next_event().await.expect("stream ended unexpectedly"); + match event.msg { + EventMsg::TurnComplete(_) => return Ok(()), + EventMsg::Error(error) => { + return Err(format!( + "review failed before completing the turn: {error:#?}" + )); + } + EventMsg::TurnAborted(aborted) => { + return Err(format!( + "review aborted before completing the turn: {aborted:#?}" + )); + } + _ => {} + } + } }) .await - .unwrap_or_else(|error| panic!("{error}")); + .unwrap_or_else(|_| { + panic!( + "timeout after entering review mode; mock response requests observed: {}", + request_log.requests().len() + ) + }); + completed_turn.unwrap_or_else(|error| panic!("{error}")); let requests = request_log.requests(); assert_eq!(requests.len(), 1); From d43fab946464264a052f3e62f2af11f327d53502 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 04:33:41 -0700 Subject: [PATCH 4/8] fix(git-utils): reuse runner for merge-base --- codex-rs/git-utils/src/branch.rs | 119 ++++++++++++++++++++++---- codex-rs/git-utils/src/git_command.rs | 17 ++++ codex-rs/git-utils/src/operations.rs | 53 ++++++++++-- 3 files changed, 165 insertions(+), 24 deletions(-) diff --git a/codex-rs/git-utils/src/branch.rs b/codex-rs/git-utils/src/branch.rs index fd5d1d05a1..13cad88f7b 100644 --- a/codex-rs/git-utils/src/branch.rs +++ b/codex-rs/git-utils/src/branch.rs @@ -2,10 +2,11 @@ use std::ffi::OsString; use std::path::Path; use crate::GitToolingError; +use crate::git_command::GitRunner; use crate::operations::ensure_git_repository; use crate::operations::resolve_head; use crate::operations::resolve_repository_root; -use crate::operations::run_git_for_stdout; +use crate::operations::run_git_for_stdout_with_runner; /// Returns the merge-base commit between `HEAD` and the latest version between local /// and remote of the provided branch, if both exist. @@ -16,25 +17,28 @@ pub fn merge_base_with_head( repo_path: &Path, branch: &str, ) -> Result, GitToolingError> { - ensure_git_repository(repo_path)?; - let repo_root = resolve_repository_root(repo_path)?; - let head = match resolve_head(repo_root.as_path())? { + let git = GitRunner::for_cwd_io(repo_path)?; + ensure_git_repository(&git, repo_path)?; + let repo_root = resolve_repository_root(&git, repo_path)?; + let head = match resolve_head(&git, repo_root.as_path())? { Some(head) => head, None => return Ok(None), }; - let Some(branch_ref) = resolve_branch_ref(repo_root.as_path(), branch)? else { + let Some(branch_ref) = resolve_branch_ref(&git, repo_root.as_path(), branch)? else { return Ok(None); }; - let preferred_ref = - if let Some(upstream) = resolve_upstream_if_remote_ahead(repo_root.as_path(), branch)? { - resolve_branch_ref(repo_root.as_path(), &upstream)?.unwrap_or(branch_ref) - } else { - branch_ref - }; + let preferred_ref = if let Some(upstream) = + resolve_upstream_if_remote_ahead(&git, repo_root.as_path(), branch)? + { + resolve_branch_ref(&git, repo_root.as_path(), &upstream)?.unwrap_or(branch_ref) + } else { + branch_ref + }; - let merge_base = run_git_for_stdout( + let merge_base = run_git_for_stdout_with_runner( + &git, repo_root.as_path(), vec![ OsString::from("merge-base"), @@ -47,8 +51,13 @@ pub fn merge_base_with_head( Ok(Some(merge_base)) } -fn resolve_branch_ref(repo_root: &Path, branch: &str) -> Result, GitToolingError> { - let rev = run_git_for_stdout( +fn resolve_branch_ref( + git: &GitRunner, + repo_root: &Path, + branch: &str, +) -> Result, GitToolingError> { + let rev = run_git_for_stdout_with_runner( + git, repo_root, vec![ OsString::from("rev-parse"), @@ -66,10 +75,12 @@ fn resolve_branch_ref(repo_root: &Path, branch: &str) -> Result, } fn resolve_upstream_if_remote_ahead( + git: &GitRunner, repo_root: &Path, branch: &str, ) -> Result, GitToolingError> { - let upstream = match run_git_for_stdout( + let upstream = match run_git_for_stdout_with_runner( + git, repo_root, vec![ OsString::from("rev-parse"), @@ -90,7 +101,8 @@ fn resolve_upstream_if_remote_ahead( Err(other) => return Err(other), }; - let counts = match run_git_for_stdout( + let counts = match run_git_for_stdout_with_runner( + git, repo_root, vec![ OsString::from("rev-list"), @@ -120,7 +132,13 @@ fn resolve_upstream_if_remote_ahead( mod tests { use super::merge_base_with_head; use crate::GitToolingError; + use crate::git_command::GitRunner; + use crate::git_command::git_runner_construction_count; + use crate::git_command::reset_git_runner_construction_count; + use crate::operations::ensure_git_repository; + use crate::operations::resolve_repository_root; use pretty_assertions::assert_eq; + use std::io; use std::path::Path; use std::process::Command; use tempfile::tempdir; @@ -187,8 +205,14 @@ mod tests { run_git_in(repo, &["checkout", "feature"]); let expected = run_git_stdout(repo, &["merge-base", "HEAD", "main"]); + reset_git_runner_construction_count(); let merge_base = merge_base_with_head(repo, "main")?; assert_eq!(merge_base, Some(expected)); + assert_eq!( + git_runner_construction_count(), + 1, + "one high-level merge-base call must retain one Git runner" + ); Ok(()) } @@ -253,4 +277,67 @@ mod tests { Ok(()) } + + #[test] + fn merge_base_returns_none_when_head_is_unborn() -> Result<(), GitToolingError> { + let temp = tempdir()?; + let repo = temp.path(); + init_test_repo(repo); + + let merge_base = merge_base_with_head(repo, "main")?; + assert_eq!(merge_base, None); + + Ok(()) + } + + #[test] + fn merge_base_reports_non_repository() { + let temp = tempdir().expect("tempdir"); + let path = temp.path(); + + let error = merge_base_with_head(path, "main").expect_err("non-repository must fail"); + let GitToolingError::NotAGitRepository { path: error_path } = error else { + panic!("expected non-repository error, got {error:?}"); + }; + assert_eq!(error_path, path); + } + + #[cfg(unix)] + #[test] + fn retained_runner_refuses_gitdir_retarget_between_subcommands() { + use std::os::unix::fs::symlink; + + let fixture = tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let admin = fixture.path().join("external-admin"); + std::fs::create_dir_all(&repo).expect("create repository"); + run_git_in( + fixture.path(), + &[ + "init", + "--separate-git-dir", + admin.to_str().expect("UTF-8 admin path"), + repo.to_str().expect("UTF-8 repository path"), + ], + ); + + reset_git_runner_construction_count(); + let git = GitRunner::for_cwd(&repo).expect("operation-scoped Git runner"); + ensure_git_repository(&git, &repo).expect("first merge-base subcommand"); + + symlink(&admin, repo.join("switch")).expect("Git-dir route switch"); + std::fs::write(repo.join(".git"), "gitdir: switch\n").expect("retarget Git-dir marker"); + + let error = resolve_repository_root(&git, &repo) + .expect_err("retained authority must refuse the second subcommand"); + let GitToolingError::Io(error) = error else { + panic!("expected metadata revalidation error, got {error:?}"); + }; + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied, "{error}"); + assert_eq!( + git_runner_construction_count(), + 1, + "subcommands must not rediscover authority after metadata retargeting" + ); + } } diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index e78420b842..924ed32385 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -61,6 +61,8 @@ impl GitCommand { impl GitRunner { pub(crate) fn for_cwd(cwd: &Path) -> Result { + #[cfg(test)] + GIT_RUNNER_CONSTRUCTION_COUNT.with(|count| count.set(count.get() + 1)); let authority = repository_authority_for_cwd(cwd)?; let search_path = std::env::var_os("PATH").ok_or(GitReadError::NoTrustedGit)?; Self::from_search_path(authority, &search_path) @@ -125,6 +127,21 @@ impl GitRunner { } } +#[cfg(test)] +thread_local! { + static GIT_RUNNER_CONSTRUCTION_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_git_runner_construction_count() { + GIT_RUNNER_CONSTRUCTION_COUNT.with(|count| count.set(0)); +} + +#[cfg(test)] +pub(crate) fn git_runner_construction_count() -> usize { + GIT_RUNNER_CONSTRUCTION_COUNT.with(std::cell::Cell::get) +} + pub(crate) fn repository_authority_for_cwd( cwd: &Path, ) -> Result { diff --git a/codex-rs/git-utils/src/operations.rs b/codex-rs/git-utils/src/operations.rs index 27c3e17ec4..24123d1ea4 100644 --- a/codex-rs/git-utils/src/operations.rs +++ b/codex-rs/git-utils/src/operations.rs @@ -7,8 +7,9 @@ use crate::GitToolingError; use crate::git_command::GitRunner; use crate::safe_git::DISABLED_HOOKS_PATH; -pub(crate) fn ensure_git_repository(path: &Path) -> Result<(), GitToolingError> { - match run_git_for_stdout( +pub(crate) fn ensure_git_repository(git: &GitRunner, path: &Path) -> Result<(), GitToolingError> { + match run_git_for_stdout_with_runner( + git, path, vec![ OsString::from("rev-parse"), @@ -29,8 +30,12 @@ pub(crate) fn ensure_git_repository(path: &Path) -> Result<(), GitToolingError> } } -pub(crate) fn resolve_head(path: &Path) -> Result, GitToolingError> { - match run_git_for_stdout( +pub(crate) fn resolve_head( + git: &GitRunner, + path: &Path, +) -> Result, GitToolingError> { + match run_git_for_stdout_with_runner( + git, path, vec![ OsString::from("rev-parse"), @@ -45,8 +50,12 @@ pub(crate) fn resolve_head(path: &Path) -> Result, GitToolingErro } } -pub(crate) fn resolve_repository_root(path: &Path) -> Result { - let root = run_git_for_stdout( +pub(crate) fn resolve_repository_root( + git: &GitRunner, + path: &Path, +) -> Result { + let root = run_git_for_stdout_with_runner( + git, path, vec![ OsString::from("rev-parse"), @@ -70,6 +79,7 @@ where Ok(()) } +#[cfg(test)] pub(crate) fn run_git_for_stdout( dir: &Path, args: I, @@ -79,7 +89,21 @@ where I: IntoIterator, S: AsRef, { - let run = run_git(dir, args, env)?; + let git = GitRunner::for_cwd_io(dir)?; + run_git_for_stdout_with_runner(&git, dir, args, env) +} + +pub(crate) fn run_git_for_stdout_with_runner( + git: &GitRunner, + dir: &Path, + args: I, + env: Option<&[(OsString, OsString)]>, +) -> Result +where + I: IntoIterator, + S: AsRef, +{ + let run = run_git_with_runner(git, dir, args, env)?; String::from_utf8(run.output.stdout) .map(|value| value.trim().to_string()) .map_err(|source| GitToolingError::GitOutputUtf8 { @@ -93,6 +117,20 @@ fn run_git( args: I, env: Option<&[(OsString, OsString)]>, ) -> Result +where + I: IntoIterator, + S: AsRef, +{ + let git = GitRunner::for_cwd_io(dir)?; + run_git_with_runner(&git, dir, args, env) +} + +fn run_git_with_runner( + git: &GitRunner, + dir: &Path, + args: I, + env: Option<&[(OsString, OsString)]>, +) -> Result where I: IntoIterator, S: AsRef, @@ -109,7 +147,6 @@ where args_vec.push(OsString::from(arg.as_ref())); } let command_string = build_command_string(&args_vec); - let git = GitRunner::for_cwd_io(dir)?; let mut command = git.command_for_cwd(dir)?; if let Some(envs) = env { for (key, value) in envs { From 10948913cd50a6790da012da5d02993befe4dd97 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 04:48:32 -0700 Subject: [PATCH 5/8] test(git-utils): gate Unix-only merge-base imports --- codex-rs/git-utils/src/branch.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/codex-rs/git-utils/src/branch.rs b/codex-rs/git-utils/src/branch.rs index 13cad88f7b..8d5c8f1e03 100644 --- a/codex-rs/git-utils/src/branch.rs +++ b/codex-rs/git-utils/src/branch.rs @@ -132,12 +132,16 @@ fn resolve_upstream_if_remote_ahead( mod tests { use super::merge_base_with_head; use crate::GitToolingError; + #[cfg(unix)] use crate::git_command::GitRunner; use crate::git_command::git_runner_construction_count; use crate::git_command::reset_git_runner_construction_count; + #[cfg(unix)] use crate::operations::ensure_git_repository; + #[cfg(unix)] use crate::operations::resolve_repository_root; use pretty_assertions::assert_eq; + #[cfg(unix)] use std::io; use std::path::Path; use std::process::Command; From 30d7a7625432969c22dfed15a013ab2872d65959 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 06:16:32 -0700 Subject: [PATCH 6/8] git-utils: scope config authorization to apply operations --- codex-rs/git-utils/src/apply.rs | 180 +++++--- codex-rs/git-utils/src/apply_filter_tests.rs | 32 ++ codex-rs/git-utils/src/errors.rs | 6 +- codex-rs/git-utils/src/filter_sentinel.rs | 1 + codex-rs/git-utils/src/git_command.rs | 17 + codex-rs/git-utils/src/git_command_tests.rs | 95 ++++ .../git-utils/src/git_config_environment.rs | 136 ++++++ .../src/git_config_environment_tests.rs | 126 +++++ codex-rs/git-utils/src/git_config_sources.rs | 2 +- .../src/git_config_sources/primary_sources.rs | 62 +-- codex-rs/git-utils/src/guarded_config.rs | 434 ++++++++++++++++++ .../git-utils/src/guarded_config_tests.rs | 116 +++++ codex-rs/git-utils/src/lib.rs | 2 + codex-rs/git-utils/src/patch_paths.rs | 135 +++++- .../src/repository_authority/authority.rs | 17 + codex-rs/git-utils/src/safe_git.rs | 258 +++-------- codex-rs/git-utils/src/safe_git_tests.rs | 74 +-- 17 files changed, 1364 insertions(+), 329 deletions(-) create mode 100644 codex-rs/git-utils/src/git_config_environment.rs create mode 100644 codex-rs/git-utils/src/git_config_environment_tests.rs create mode 100644 codex-rs/git-utils/src/guarded_config.rs create mode 100644 codex-rs/git-utils/src/guarded_config_tests.rs diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index d9c68fc2ae..5f3186a70d 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -13,11 +13,10 @@ use std::path::PathBuf; use crate::FsmonitorOverride; use crate::apply_output::parse_git_apply_output; use crate::git_command::GitRunner; -use crate::git_config_sources::ensure_no_worktree_config_sources; -use crate::patch_paths::extract_effective_paths_from_patch; -use crate::patch_paths::stage_effective_paths; +use crate::guarded_config::GuardedGitConfig; +use crate::patch_paths::extract_effective_paths_from_patch_guarded; +use crate::patch_paths::stage_effective_paths_guarded; use crate::safe_git::DISABLED_HOOKS_PATH; -use crate::safe_git::ensure_no_selected_executable_git_filters; #[cfg(test)] use crate::safe_git::isolate_git_command_environment; @@ -47,29 +46,27 @@ pub struct ApplyGitResult { /// When [`ApplyGitRequest::preflight`] is `true`, this behaves like `git apply --check` and /// leaves the working tree untouched while still parsing the command output for diagnostics. pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { + let cfg_parts = configured_git_config_parts(); + // Construct from the caller's exact route before canonicalization so + // repository authority retains lexical enclosing worktrees. let git = GitRunner::for_cwd_io(&req.cwd)?; - let mut cfg_parts = configured_git_config_parts(); - ensure_no_worktree_config_sources(&git, &req.cwd, &cfg_parts)?; let requested_cwd = std::fs::canonicalize(&req.cwd)?; - let git_root = resolve_git_root(&git, &req.cwd, &cfg_parts)?; - if git_root != requested_cwd { - ensure_no_worktree_config_sources(&git, &git_root, &cfg_parts)?; - } + let expected_root = crate::get_git_repo_root(&requested_cwd) + .ok_or_else(|| io::Error::other("not a Git repository")) + .and_then(std::fs::canonicalize)?; + let mut config = GuardedGitConfig::authorize(&git, &expected_root, cfg_parts)?; + resolve_git_root(&config, &requested_cwd)?; // Write unified diff into a temporary file let (tmpdir, patch_path) = write_temp_patch(&req.diff)?; // Keep tmpdir alive until function end to ensure the file exists let _guard = tmpdir; - let patch_paths = - extract_effective_paths_from_patch(&git, &git_root, &patch_path, req.revert, &cfg_parts)?; - let filter_guard = - ensure_no_selected_executable_git_filters(&git, &git_root, &patch_paths, &cfg_parts)?; - cfg_parts.extend(safe_git_config_parts()); - cfg_parts.extend_from_slice(filter_guard.git_config_args()); + let patch_paths = extract_effective_paths_from_patch_guarded(&config, &patch_path, req.revert)?; + config.authorize_filter_paths(&patch_paths)?; if req.revert && !req.preflight { // Stage WT paths first to avoid index mismatch on revert. - stage_effective_paths(&git, &git_root, &patch_paths, &cfg_parts)?; + stage_effective_paths_guarded(&config, &patch_paths)?; } // Build git args @@ -87,8 +84,8 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { check_args.push("-R".to_string()); } check_args.push(patch_path.to_string_lossy().to_string()); - let rendered = render_command_for_log(&git_root, &cfg_parts, &check_args); - let (c_code, c_out, c_err) = run_git(&git, &git_root, &cfg_parts, &check_args)?; + let rendered = config.render_command_for_log(&check_args)?; + let (c_code, c_out, c_err) = run_guarded_apply(&config, &check_args)?; let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = parse_git_apply_output(&c_out, &c_err); applied_paths.sort(); @@ -108,8 +105,8 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { }); } - 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)?; + let cmd_for_log = config.render_command_for_log(&args)?; + let (code, stdout, stderr) = run_guarded_apply(&config, &args)?; let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = parse_git_apply_output(&stdout, &stderr); @@ -131,18 +128,10 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { }) } -fn resolve_git_root( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], -) -> io::Result { - let requested_cwd = std::fs::canonicalize(cwd)?; - let mut command = git.command_for_cwd(&requested_cwd)?; - command - .args(git_config_args) - .arg("rev-parse") - .arg("--show-toplevel"); - let out = git.output(command)?; +fn resolve_git_root(config: &GuardedGitConfig<'_>, requested_cwd: &Path) -> io::Result { + let mut command = config.rev_parse_command()?; + command.arg("--show-toplevel"); + let out = command.output()?; let code = out.status.code().unwrap_or(-1); if code != 0 { return Err(io::Error::other(format!( @@ -153,18 +142,7 @@ fn resolve_git_root( } let reported_root = PathBuf::from(String::from_utf8_lossy(&out.stdout).trim()); let root = std::fs::canonicalize(&reported_root)?; - let expected_root = crate::get_git_repo_root(&requested_cwd) - .ok_or_else(|| { - io::Error::new( - io::ErrorKind::PermissionDenied, - format!( - "refusing to apply a patch because Git resolved worktree {} without a .git marker above requested cwd {}", - root.display(), - requested_cwd.display() - ), - ) - }) - .and_then(std::fs::canonicalize)?; + let expected_root = config.canonical_root(); if root != expected_root { return Err(io::Error::new( io::ErrorKind::PermissionDenied, @@ -221,6 +199,32 @@ pub(crate) fn run_git( Ok((code, stdout, stderr)) } +fn run_guarded_apply( + config: &GuardedGitConfig<'_>, + args: &[String], +) -> io::Result<(i32, String, String)> { + let Some((subcommand, args)) = args.split_first() else { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "missing guarded Git subcommand", + )); + }; + if subcommand != "apply" { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "unexpected guarded Git subcommand", + )); + } + let mut command = config.apply_command()?; + command.args(args); + let output = command.output()?; + Ok(( + output.status.code().unwrap_or(-1), + String::from_utf8_lossy(&output.stdout).into_owned(), + String::from_utf8_lossy(&output.stderr).into_owned(), + )) +} + pub(crate) fn safe_git_config_parts() -> Vec { vec![ "-c".to_string(), @@ -230,33 +234,6 @@ pub(crate) fn safe_git_config_parts() -> Vec { ] } -fn quote_shell(s: &str) -> String { - let simple = s - .chars() - .all(|c| c.is_ascii_alphanumeric() || "-_.:/@%+".contains(c)); - if simple { - s.to_string() - } else { - format!("'{}'", s.replace('\'', "'\\''")) - } -} - -fn render_command_for_log(cwd: &Path, git_cfg: &[String], args: &[String]) -> String { - let mut parts: Vec = Vec::new(); - parts.push("git".to_string()); - for a in git_cfg { - parts.push(quote_shell(a)); - } - for a in args { - parts.push(quote_shell(a)); - } - format!( - "(cd {} && {})", - quote_shell(&cwd.display().to_string()), - parts.join(" ") - ) -} - #[cfg(all(test, unix))] #[path = "apply_transport_tests.rs"] mod transport_tests; @@ -268,6 +245,8 @@ mod filter_tests; #[cfg(test)] mod tests { use super::*; + use crate::guarded_config::config_source_authorization_count; + use crate::guarded_config::reset_config_source_authorization_count; use std::ffi::OsStr; use std::path::Path; use std::sync::Mutex; @@ -412,7 +391,9 @@ mod tests { revert: false, preflight: false, }; + reset_config_source_authorization_count(); let r = apply_git_patch(&req).expect("run apply"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(r.exit_code, 0, "exit code 0"); // File exists now assert!(root.join("hello.txt").exists()); @@ -541,6 +522,41 @@ mod tests { assert!(!marker.exists(), "enclosing Git shim must not run"); } + #[cfg(unix)] + #[test] + fn apply_uses_physical_repository_for_symlinked_nested_cwd() { + let _g = env_lock().lock().unwrap(); + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let target = fixture.path().join("target"); + let nested = target.join("nested"); + std::fs::create_dir_all(&outer).expect("outer"); + std::fs::create_dir_all(&nested).expect("nested"); + let (outer_init, _, outer_error) = run(&outer, &["git", "init", "-q"]); + assert_eq!(outer_init, 0, "outer init: {outer_error}"); + let (target_init, _, target_error) = run(&target, &["git", "init", "-q"]); + assert_eq!(target_init, 0, "target init: {target_error}"); + let lexical_cwd = outer.join("linked-nested"); + std::os::unix::fs::symlink(&nested, &lexical_cwd).expect("nested cwd symlink"); + + reset_config_source_authorization_count(); + let result = apply_git_patch(&ApplyGitRequest { + cwd: lexical_cwd, + diff: "diff --git a/physical.txt b/physical.txt\nnew file mode 100644\n--- /dev/null\n+++ b/physical.txt\n@@ -0,0 +1 @@\n+physical\n".to_string(), + revert: false, + preflight: false, + }) + .expect("apply through symlinked nested cwd"); + + assert_eq!(result.exit_code, 0); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!( + read_file_normalized(&target.join("physical.txt")), + "physical\n" + ); + assert!(!outer.join("physical.txt").exists()); + } + #[test] fn apply_modify_conflict() { let _g = env_lock().lock().unwrap(); @@ -599,7 +615,9 @@ mod tests { revert: false, preflight: false, }; + reset_config_source_authorization_count(); let res_apply = apply_git_patch(&apply_req).expect("apply ok"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(res_apply.exit_code, 0, "forward apply succeeded"); let after_apply = read_file_normalized(&root.join("file.txt")); assert_eq!(after_apply, "ORIG\n"); @@ -611,7 +629,9 @@ mod tests { revert: true, preflight: false, }; + reset_config_source_authorization_count(); let res_revert = apply_git_patch(&revert_req).expect("revert ok"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(res_revert.exit_code, 0, "revert apply succeeded"); let after_revert = read_file_normalized(&root.join("file.txt")); assert_eq!(after_revert, "orig\n"); @@ -648,7 +668,9 @@ mod tests { revert: true, preflight: true, }; + reset_config_source_authorization_count(); let res_preflight = apply_git_patch(&preflight_req).expect("preflight ok"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(res_preflight.exit_code, 0, "revert preflight succeeded"); let (_code_after, staged_after, _stderr_after) = run(root, &["git", "diff", "--cached", "--name-only"]); @@ -678,7 +700,9 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 revert: false, preflight: true, }; + reset_config_source_authorization_count(); let r1 = apply_git_patch(&req1).expect("preflight apply"); + assert_eq!(config_source_authorization_count(), 1); assert_ne!(r1.exit_code, 0, "preflight reports failure"); assert!( !root.join("ok.txt").exists(), @@ -696,7 +720,9 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 revert: false, preflight: false, }; + reset_config_source_authorization_count(); let r2 = apply_git_patch(&req2).expect("direct apply"); + assert_eq!(config_source_authorization_count(), 1); assert_ne!(r2.exit_code, 0, "apply is expected to fail overall"); assert!( !r2.cmd_for_log.contains("--check"), @@ -724,7 +750,13 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 revert, preflight, }; + reset_config_source_authorization_count(); let error = apply_git_patch(&request).expect_err("reject configured filter"); + assert_eq!( + config_source_authorization_count(), + 1, + "{revert} {preflight}" + ); assert_eq!(error.kind(), io::ErrorKind::Unsupported); assert!(!configured_filter_ran(root)); assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n"); @@ -812,8 +844,10 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 assert_eq!(config_code, 0, "configure core.worktree: {config_err}"); let git = GitRunner::for_cwd_io(&attacker).expect("trusted Git"); + let guarded = GuardedGitConfig::authorize(&git, &attacker, Vec::new()) + .expect("authorize attacker repository config"); let error = - resolve_git_root(&git, &attacker, &[]).expect_err("reject redirected worktree"); + resolve_git_root(&guarded, &attacker).expect_err("reject redirected worktree"); assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); assert!(error.to_string().contains("instead of expected worktree")); } diff --git a/codex-rs/git-utils/src/apply_filter_tests.rs b/codex-rs/git-utils/src/apply_filter_tests.rs index 12da828fa6..d2418d4a03 100644 --- a/codex-rs/git-utils/src/apply_filter_tests.rs +++ b/codex-rs/git-utils/src/apply_filter_tests.rs @@ -553,6 +553,38 @@ fn command_scoped_offpath_filter_is_neutralized_after_local_config() { ); } +#[test] +fn filter_override_rejects_process_temp_directory_inside_worktree() { + let root_name = "CODEX_GIT_UTILS_WORKTREE_TMP_ROOT"; + if let Some(root) = std::env::var_os(root_name) { + let root = std::path::PathBuf::from(root); + let error = apply_git_patch(&request( + &root, /*revert*/ false, /*preflight*/ true, + )) + .expect_err("reject worktree-owned filter override"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + assert!(!configured_filter_ran(&root)); + return; + } + + let repo = init_repo(); + let root = repo.path(); + configure_filter(root, "demo"); + let worktree_temp = root.join("process-temp"); + std::fs::create_dir(&worktree_temp).expect("worktree temp directory"); + run_isolated_test( + "apply::filter_tests::filter_override_rejects_process_temp_directory_inside_worktree", + &[ + (root_name, root.as_os_str()), + ("TMPDIR", worktree_temp.as_os_str()), + #[cfg(windows)] + ("TEMP", worktree_temp.as_os_str()), + #[cfg(windows)] + ("TMP", worktree_temp.as_os_str()), + ], + ); +} + #[test] fn apply_neutralizes_filter_activated_for_racy_offpath_by_same_patch() { for (name, command) in [ diff --git a/codex-rs/git-utils/src/errors.rs b/codex-rs/git-utils/src/errors.rs index 574cfc311a..04274135ec 100644 --- a/codex-rs/git-utils/src/errors.rs +++ b/codex-rs/git-utils/src/errors.rs @@ -50,6 +50,8 @@ pub(crate) enum GitReadError { InvalidRepositoryMetadata { path: PathBuf, reason: String }, #[error("{path:?} is not a Git repository")] NotRepository { path: PathBuf }, + #[error("invalid Git configuration environment: {reason}")] + InvalidConfigEnvironment { reason: String }, } impl GitReadError { @@ -59,7 +61,9 @@ impl GitReadError { Self::UnprovenPrimaryAuthority { .. } | Self::UnsafeRepositoryMetadata { .. } => { std::io::ErrorKind::PermissionDenied } - Self::InvalidRepositoryMetadata { .. } => std::io::ErrorKind::InvalidData, + Self::InvalidRepositoryMetadata { .. } | Self::InvalidConfigEnvironment { .. } => { + std::io::ErrorKind::InvalidData + } } } diff --git a/codex-rs/git-utils/src/filter_sentinel.rs b/codex-rs/git-utils/src/filter_sentinel.rs index 7847ac3e63..dc3cd1ee56 100644 --- a/codex-rs/git-utils/src/filter_sentinel.rs +++ b/codex-rs/git-utils/src/filter_sentinel.rs @@ -52,6 +52,7 @@ pub(crate) fn classify_sentinel_filter_probes( } } +#[cfg(test)] pub(crate) fn sentinel_filter_probe_config_args( neutralization_args: &[String], driver: &str, diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 7e637664de..d51b44d04b 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -6,6 +6,7 @@ use std::process::Command; use std::process::Stdio; use crate::errors::GitReadError; +use crate::git_config_environment::GitConfigEnvironmentSnapshot; #[cfg(test)] use crate::git_executable::git_executable_name; use crate::git_executable::harden_git_launch_environment; @@ -31,6 +32,7 @@ pub(crate) struct GitRunner { argv0: PathBuf, safe_path: std::ffi::OsString, authority: RepositoryAuthority, + config_environment: GitConfigEnvironmentSnapshot, } /// A Git command that can only be spawned through [`GitRunner::output`], @@ -95,6 +97,7 @@ impl GitRunner { command.current_dir(parent); } harden_git_launch_environment(&mut command, &self.safe_path); + self.config_environment.apply_to(&mut command); GitCommand { inner: command } } @@ -119,6 +122,14 @@ impl GitRunner { .ensure_config_source_is_not_worktree_controlled(path, description) } + pub(crate) fn ensure_active_worktree_root(&self, root: &Path) -> io::Result<()> { + self.authority.ensure_active_worktree_root(root) + } + + pub(crate) fn config_environment_value(&self, name: &str) -> Option<&OsStr> { + self.config_environment.value(name) + } + pub(crate) fn output(&self, mut command: GitCommand) -> io::Result { self.revalidate_active_repository_metadata()?; isolate_git_command_environment(&mut command.inner); @@ -137,12 +148,18 @@ impl GitRunner { ) -> Result { authority.ensure_primary_authority()?; let selected = select_git_executable(&authority, search_path)?; + let config_environment = GitConfigEnvironmentSnapshot::capture().map_err(|error| { + GitReadError::InvalidConfigEnvironment { + reason: error.to_string(), + } + })?; Ok(Self { executable: selected.executable, #[cfg(any(unix, test))] argv0: selected.argv0, safe_path: selected.safe_path, authority, + config_environment, }) } } diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index cf6e1b6f46..3d191735b3 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -52,6 +52,95 @@ fn run_git_stdout(cwd: &Path, args: &[&str]) -> String { .to_string() } +#[test] +fn runner_binds_config_environment_across_ambient_mutation() { + const CHILD: &str = "CODEX_GIT_CONFIG_BINDING_CHILD"; + if std::env::var_os(CHILD).is_none() { + let fixture = tempfile::tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let safe_home = fixture.path().join("safe-home"); + let malicious_home = repo.join("malicious-home"); + std::fs::create_dir_all(&repo).expect("repo"); + std::fs::create_dir_all(&safe_home).expect("safe home"); + std::fs::create_dir_all(&malicious_home).expect("malicious home"); + run_git(&repo, &["init", "-q"]); + std::fs::write( + safe_home.join(".gitconfig"), + "[codex]\n\tbound = safe-home\n", + ) + .expect("safe home config"); + let malicious_global = repo.join("malicious.gitconfig"); + std::fs::write(&malicious_global, "[codex]\n\tbound = malicious\n") + .expect("malicious global config"); + std::fs::write( + malicious_home.join(".gitconfig"), + "[codex]\n\tbound = malicious-home\n", + ) + .expect("malicious home config"); + + let mut command = Command::new(std::env::current_exe().expect("test executable")); + isolate_git_command_environment(&mut command); + let output = command + .arg("git_command::tests::runner_binds_config_environment_across_ambient_mutation") + .arg("--exact") + .arg("--nocapture") + .env(CHILD, "1") + .env("RUST_TEST_THREADS", "1") + .env("CODEX_GIT_CONFIG_BINDING_ROOT", &repo) + .env("CODEX_GIT_CONFIG_BINDING_MALICIOUS", &malicious_global) + .env("CODEX_GIT_CONFIG_BINDING_MALICIOUS_HOME", &malicious_home) + .env("HOME", &safe_home) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "1") + .env("GIT_CONFIG_KEY_0", "codex.count") + .env("GIT_CONFIG_VALUE_0", "safe-count") + .env_remove("GIT_CONFIG_GLOBAL") + .env_remove("GIT_CONFIG_SYSTEM") + .env_remove("GIT_CONFIG_PARAMETERS") + .output() + .expect("isolated config binding test"); + assert!( + output.status.success(), + "isolated config binding test failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + let root = PathBuf::from(std::env::var_os("CODEX_GIT_CONFIG_BINDING_ROOT").expect("root")); + let malicious = + std::env::var_os("CODEX_GIT_CONFIG_BINDING_MALICIOUS").expect("malicious config"); + let malicious_home = + std::env::var_os("CODEX_GIT_CONFIG_BINDING_MALICIOUS_HOME").expect("malicious home"); + let runner = GitRunner::for_cwd_io(&root).expect("runner with captured config environment"); + + // SAFETY: the parent starts an isolated test process that runs only this + // exact test with one harness thread. No other application thread reads or + // writes these variables, and the process exits after the assertions. + unsafe { + std::env::set_var("GIT_CONFIG_GLOBAL", malicious); + std::env::set_var("HOME", malicious_home); + std::env::set_var("GIT_CONFIG_VALUE_0", "malicious-count"); + std::env::set_var( + "GIT_CONFIG_PARAMETERS", + "'codex.count'='malicious-parameter'", + ); + } + + for (key, expected) in [("codex.bound", "safe-home"), ("codex.count", "safe-count")] { + let mut command = runner.command_for_cwd(&root).expect("bound command"); + command.args(["config", "--get", key]); + let output = runner.output(command).expect("bound config read"); + assert!( + output.status.success(), + "{key}: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), expected); + } +} + fn commit_all(cwd: &Path, message: &str) { run_git( cwd, @@ -505,6 +594,12 @@ fn git_read_error_io_kind_table_is_exhaustive() { }, io::ErrorKind::InvalidData, ), + ( + GitReadError::InvalidConfigEnvironment { + reason: "malformed GIT_CONFIG_COUNT".to_string(), + }, + io::ErrorKind::InvalidData, + ), ] { assert_eq!(error.io_kind(), expected, "{error}"); assert_eq!(error.into_io_error().kind(), expected); diff --git a/codex-rs/git-utils/src/git_config_environment.rs b/codex-rs/git-utils/src/git_config_environment.rs new file mode 100644 index 0000000000..43d80a3afd --- /dev/null +++ b/codex-rs/git-utils/src/git_config_environment.rs @@ -0,0 +1,136 @@ +use std::collections::BTreeMap; +use std::ffi::OsStr; +use std::ffi::OsString; +use std::io; +use std::process::Command; + +const FIXED_CONFIG_ENVIRONMENT_KEYS: &[&str] = &[ + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_SYSTEM", + "GIT_CONFIG_NOSYSTEM", + "GIT_CONFIG_COUNT", + "GIT_CONFIG_PARAMETERS", + "HOME", + "XDG_CONFIG_HOME", + #[cfg(windows)] + "APPDATA", + #[cfg(windows)] + "PROGRAMDATA", + #[cfg(windows)] + "USERPROFILE", + #[cfg(windows)] + "HOMEDRIVE", + #[cfg(windows)] + "HOMEPATH", +]; + +// Git config command entries are an untrusted process input. Keep capture +// bounded rather than allocating attacker-selected amounts of memory before +// Git gets a chance to reject an unreasonable count. +const MAX_CONFIG_ENVIRONMENT_ENTRIES: usize = 1024; + +/// Exact config-relevant process environment bound to one +/// [`GitRunner`](crate::git_command::GitRunner). +/// +/// Every ordinary Git child receives these captured values (including explicit +/// removals for variables that were absent). Commands that intentionally +/// isolate a config source may override them after command construction. +pub(crate) struct GitConfigEnvironmentSnapshot { + entries: Box<[(OsString, Option)]>, +} + +impl std::fmt::Debug for GitConfigEnvironmentSnapshot { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("GitConfigEnvironmentSnapshot") + .field("captured_entries", &self.entries.len()) + .finish() + } +} + +impl GitConfigEnvironmentSnapshot { + pub(crate) fn capture() -> io::Result { + let environment = std::env::vars_os().collect::>(); + Self::capture_from(|name| captured_environment_value(&environment, name)) + } + + fn capture_from(mut value_for: impl FnMut(&OsStr) -> Option) -> io::Result { + let mut entries = FIXED_CONFIG_ENVIRONMENT_KEYS + .iter() + .map(|name| (OsString::from(name), value_for(OsStr::new(name)))) + .collect::>(); + + if let Some(raw_count) = value_for(OsStr::new("GIT_CONFIG_COUNT")) { + let count = raw_count + .to_str() + .ok_or_else(|| invalid_environment("non-UTF-8 GIT_CONFIG_COUNT"))? + .parse::() + .map_err(|_| invalid_environment("invalid GIT_CONFIG_COUNT"))?; + if count > MAX_CONFIG_ENVIRONMENT_ENTRIES { + return Err(invalid_environment( + "GIT_CONFIG_COUNT exceeds the supported safety bound", + )); + } + for index in 0..count { + for prefix in ["GIT_CONFIG_KEY_", "GIT_CONFIG_VALUE_"] { + let name = OsString::from(format!("{prefix}{index}")); + let value = value_for(&name); + entries.push((name, value)); + } + } + } + + Ok(Self { + entries: entries.into_boxed_slice(), + }) + } + + pub(crate) fn apply_to(&self, command: &mut Command) { + for (name, value) in &self.entries { + match value { + Some(value) => { + command.env(name, value); + } + None => { + command.env_remove(name); + } + } + } + } + + pub(crate) fn value(&self, name: &str) -> Option<&OsStr> { + self.entries + .iter() + .find_map(|(candidate, value)| (candidate == name).then_some(value.as_deref())) + .flatten() + } +} + +#[cfg(not(windows))] +fn captured_environment_value( + environment: &BTreeMap, + name: &OsStr, +) -> Option { + environment.get(name).cloned() +} + +#[cfg(windows)] +fn captured_environment_value( + environment: &BTreeMap, + name: &OsStr, +) -> Option { + environment.iter().find_map(|(candidate, value)| { + candidate + .to_string_lossy() + .eq_ignore_ascii_case(&name.to_string_lossy()) + .then(|| value.clone()) + }) +} + +fn invalid_environment(message: &str) -> io::Error { + io::Error::new(io::ErrorKind::InvalidData, message) +} + +#[cfg(test)] +#[path = "git_config_environment_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/git_config_environment_tests.rs b/codex-rs/git-utils/src/git_config_environment_tests.rs new file mode 100644 index 0000000000..3cdec9a790 --- /dev/null +++ b/codex-rs/git-utils/src/git_config_environment_tests.rs @@ -0,0 +1,126 @@ +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn snapshot_binds_present_empty_and_absent_fixed_and_indexed_values() { + let values = BTreeMap::from([ + (OsString::from("GIT_CONFIG_GLOBAL"), OsString::from("")), + ( + OsString::from("GIT_CONFIG_PARAMETERS"), + OsString::from("'safe.parameter'='present'"), + ), + (OsString::from("GIT_CONFIG_NOSYSTEM"), OsString::from("1")), + (OsString::from("HOME"), OsString::from("/safe/home")), + ( + OsString::from("XDG_CONFIG_HOME"), + OsString::from("/safe/xdg"), + ), + (OsString::from("GIT_CONFIG_COUNT"), OsString::from("2")), + ( + OsString::from("GIT_CONFIG_KEY_0"), + OsString::from("safe.one"), + ), + ( + OsString::from("GIT_CONFIG_VALUE_0"), + OsString::from("present"), + ), + ( + OsString::from("GIT_CONFIG_KEY_1"), + OsString::from("safe.two"), + ), + ]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| values.get(name).cloned()) + .expect("capture environment"); + + assert_eq!(snapshot.value("GIT_CONFIG_GLOBAL"), Some(OsStr::new(""))); + assert_eq!(snapshot.value("HOME"), Some(OsStr::new("/safe/home"))); + assert_eq!(snapshot.value("GIT_CONFIG_SYSTEM"), None); + assert_eq!(snapshot.value("GIT_CONFIG_NOSYSTEM"), Some(OsStr::new("1"))); + assert_eq!( + snapshot.value("GIT_CONFIG_PARAMETERS"), + Some(OsStr::new("'safe.parameter'='present'")) + ); + assert_eq!( + snapshot.value("GIT_CONFIG_KEY_0"), + Some(OsStr::new("safe.one")) + ); + assert_eq!(snapshot.value("GIT_CONFIG_VALUE_1"), None); + + let mut command = Command::new("git"); + command + .env("GIT_CONFIG_SYSTEM", "/later/system") + .env("GIT_CONFIG_VALUE_1", "later"); + snapshot.apply_to(&mut command); + let child_environment = command + .get_envs() + .map(|(name, value)| (name.to_owned(), value.map(OsStr::to_owned))) + .collect::>(); + assert_eq!( + child_environment.get(OsStr::new("GIT_CONFIG_SYSTEM")), + Some(&None) + ); + assert_eq!( + child_environment.get(OsStr::new("GIT_CONFIG_VALUE_1")), + Some(&None) + ); + assert_eq!( + child_environment.get(OsStr::new("HOME")), + Some(&Some(OsString::from("/safe/home"))) + ); +} + +#[test] +fn snapshot_rejects_malformed_or_unbounded_command_config_count() { + for count in [OsString::from("invalid"), OsString::from("1025")] { + let error = GitConfigEnvironmentSnapshot::capture_from(|name| { + (name == "GIT_CONFIG_COUNT").then(|| count.clone()) + }) + .expect_err("reject count"); + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + } +} + +#[cfg(unix)] +#[test] +fn snapshot_preserves_non_utf8_config_path_bytes() { + use std::os::unix::ffi::OsStringExt; + + let raw = OsString::from_vec(vec![b'/', b't', b'm', b'p', b'/', 0xff]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| { + (name == "GIT_CONFIG_GLOBAL").then(|| raw.clone()) + }) + .expect("capture non-UTF-8 path"); + assert_eq!(snapshot.value("GIT_CONFIG_GLOBAL"), Some(raw.as_os_str())); +} + +#[cfg(windows)] +#[test] +fn snapshot_matches_mixed_case_windows_environment_names() { + let environment = BTreeMap::from([ + (OsString::from("git_config_count"), OsString::from("1")), + ( + OsString::from("Git_Config_Key_0"), + OsString::from("safe.key"), + ), + ( + OsString::from("gIt_CoNfIg_VaLuE_0"), + OsString::from("safe-value"), + ), + (OsString::from("Home"), OsString::from(r"C:\safe-home")), + ]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| { + captured_environment_value(&environment, name) + }) + .expect("capture mixed-case Windows environment"); + + assert_eq!(snapshot.value("GIT_CONFIG_COUNT"), Some(OsStr::new("1"))); + assert_eq!( + snapshot.value("GIT_CONFIG_KEY_0"), + Some(OsStr::new("safe.key")) + ); + assert_eq!( + snapshot.value("GIT_CONFIG_VALUE_0"), + Some(OsStr::new("safe-value")) + ); + assert_eq!(snapshot.value("HOME"), Some(OsStr::new(r"C:\safe-home"))); +} diff --git a/codex-rs/git-utils/src/git_config_sources.rs b/codex-rs/git-utils/src/git_config_sources.rs index f602644151..bcfec7bdf1 100644 --- a/codex-rs/git-utils/src/git_config_sources.rs +++ b/codex-rs/git-utils/src/git_config_sources.rs @@ -36,7 +36,7 @@ pub(crate) fn ensure_no_worktree_config_sources( // Environment, HOME, and XDG paths are attacker-controlled byte strings. // Classify their exact OsString spelling before `git var` can open or // newline-delimit one of them. - for (description, candidate) in legacy_primary_config_source_candidates()? { + for (description, candidate) in legacy_primary_config_source_candidates(git)? { if !is_disabled_primary_config_path(&candidate) { reject_source(git, &git_root, &candidate, description)?; } diff --git a/codex-rs/git-utils/src/git_config_sources/primary_sources.rs b/codex-rs/git-utils/src/git_config_sources/primary_sources.rs index ed4866e510..31cea3941f 100644 --- a/codex-rs/git-utils/src/git_config_sources/primary_sources.rs +++ b/codex-rs/git-utils/src/git_config_sources/primary_sources.rs @@ -1,3 +1,4 @@ +use std::ffi::OsStr; use std::io; use std::path::Path; use std::path::PathBuf; @@ -11,7 +12,9 @@ pub(super) fn default_system_config_source_candidates( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if git_env_bool("GIT_CONFIG_NOSYSTEM")? || std::env::var_os("GIT_CONFIG_SYSTEM").is_some() { + if git_env_bool(git, "GIT_CONFIG_NOSYSTEM")? + || git.config_environment_value("GIT_CONFIG_SYSTEM").is_some() + { return Ok(Vec::new()); } // `GIT_CONFIG_SYSTEM` was added to `git var` in Git 2.42. The PSEC-4394 @@ -33,16 +36,19 @@ pub(super) fn selected_git_home_config_candidates( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if std::env::var_os("GIT_CONFIG_GLOBAL").is_some() { + if git.config_environment_value("GIT_CONFIG_GLOBAL").is_some() { return Ok(Vec::new()); } #[cfg(not(windows))] - if std::env::var_os("HOME").is_none() { + if git.config_environment_value("HOME").is_none() { return Ok(Vec::new()); } let dot_gitconfig = selected_git_path_candidate(git, cwd, "~/.gitconfig")?; let mut candidates = vec![dot_gitconfig.clone()]; - if std::env::var_os("XDG_CONFIG_HOME").is_none_or(|path| path.is_empty()) { + if git + .config_environment_value("XDG_CONFIG_HOME") + .is_none_or(OsStr::is_empty) + { let home = dot_gitconfig .parent() .ok_or_else(|| invalid_config_source("selected Git HOME has no parent"))?; @@ -55,7 +61,9 @@ pub(super) fn selected_git_prefix_system_candidate( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if git_env_bool("GIT_CONFIG_NOSYSTEM")? || std::env::var_os("GIT_CONFIG_SYSTEM").is_some() { + if git_env_bool(git, "GIT_CONFIG_NOSYSTEM")? + || git.config_environment_value("GIT_CONFIG_SYSTEM").is_some() + { return Ok(None); } selected_git_path_candidate(git, cwd, "%(prefix)/etc/gitconfig").map(Some) @@ -149,24 +157,25 @@ fn parse_git_var_paths(output: &[u8]) -> io::Result> { .collect() } -pub(super) fn legacy_primary_config_source_candidates() -> io::Result> -{ +pub(super) fn legacy_primary_config_source_candidates( + git: &GitRunner, +) -> io::Result> { let mut candidates = Vec::new(); - match std::env::var_os("GIT_CONFIG_GLOBAL") { + match git.config_environment_value("GIT_CONFIG_GLOBAL") { Some(path) if !path.is_empty() => { candidates.push(("GIT_CONFIG_GLOBAL", PathBuf::from(path))); } Some(_) => {} None => { - let homes = git_home_directories(); - match std::env::var_os("XDG_CONFIG_HOME") { + let homes = git_home_directories(git); + match git.config_environment_value("XDG_CONFIG_HOME") { Some(xdg) if !xdg.is_empty() => candidates.push(( "XDG_CONFIG_HOME Git config", PathBuf::from(xdg).join("git/config"), )), _ => { #[cfg(windows)] - if let Some(app_data) = std::env::var_os("APPDATA") + if let Some(app_data) = git.config_environment_value("APPDATA") && !app_data.is_empty() { candidates.push(( @@ -187,15 +196,15 @@ pub(super) fn legacy_primary_config_source_candidates() -> io::Result { candidates.push(("GIT_CONFIG_SYSTEM", PathBuf::from(path))); } Some(_) => {} None => { #[cfg(windows)] - if let Some(program_data) = std::env::var_os("PROGRAMDATA") + if let Some(program_data) = git.config_environment_value("PROGRAMDATA") && !program_data.is_empty() { candidates.push(( @@ -209,24 +218,25 @@ pub(super) fn legacy_primary_config_source_candidates() -> io::Result Vec { - if let Some(home) = std::env::var_os("HOME") { - return vec![home]; +fn git_home_directories(git: &GitRunner) -> Vec { + if let Some(home) = git.config_environment_value("HOME") { + return vec![home.to_owned()]; } #[cfg(windows)] { let mut homes = Vec::new(); - if let (Some(drive), Some(path)) = - (std::env::var_os("HOMEDRIVE"), std::env::var_os("HOMEPATH")) - { - let mut home = drive; + if let (Some(drive), Some(path)) = ( + git.config_environment_value("HOMEDRIVE"), + git.config_environment_value("HOMEPATH"), + ) { + let mut home = drive.to_owned(); home.push(path); homes.push(home); } - if let Some(profile) = std::env::var_os("USERPROFILE") - && !homes.contains(&profile) + if let Some(profile) = git.config_environment_value("USERPROFILE") + && !homes.iter().any(|home| home == profile) { - homes.push(profile); + homes.push(profile.to_owned()); } homes } @@ -242,8 +252,8 @@ fn home_config_path(home: &std::ffi::OsStr, suffix: &str) -> PathBuf { } } -fn git_env_bool(name: &str) -> io::Result { - let Some(value) = std::env::var_os(name) else { +fn git_env_bool(git: &GitRunner, name: &str) -> io::Result { + let Some(value) = git.config_environment_value(name) else { return Ok(false); }; let value = value diff --git a/codex-rs/git-utils/src/guarded_config.rs b/codex-rs/git-utils/src/guarded_config.rs new file mode 100644 index 0000000000..ac8e3d5321 --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config.rs @@ -0,0 +1,434 @@ +use std::collections::BTreeMap; +use std::ffi::OsStr; +use std::io; +use std::path::Path; +use std::path::PathBuf; +use std::process::Stdio; +use std::sync::Arc; + +use crate::FsmonitorOverride; +use crate::git_command::GitCommand; +use crate::git_command::GitRunner; +use crate::git_config::GitConfigEntry; +use crate::git_config::read_effective_config_with_fallback; +use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::safe_git::DISABLED_HOOKS_PATH; +use crate::safe_git::ExecutableFilterDrivers; +use crate::safe_git::FilterPolicySnapshot; +use crate::safe_git::build_filter_policy_snapshot; + +/// Proof that one exact Git config invocation has no worktree-controlled +/// source routes for one runner and canonical repository root. +/// +/// The capability deliberately owns the ordered base arguments and cannot be +/// cloned or rebound to another runner, root, or command environment. +pub(crate) struct ValidatedConfigSources<'git> { + git: &'git GitRunner, + canonical_root: PathBuf, + base_config_args: Box<[String]>, +} + +impl<'git> ValidatedConfigSources<'git> { + fn authorize( + git: &'git GitRunner, + canonical_root: &Path, + base_config_args: Vec, + ) -> io::Result { + #[cfg(test)] + CONFIG_SOURCE_AUTHORIZATION_COUNT.with(|count| count.set(count.get() + 1)); + + validate_base_config_args(&base_config_args)?; + let canonical_root = std::fs::canonicalize(canonical_root)?; + git.ensure_active_worktree_root(&canonical_root)?; + ensure_no_worktree_config_sources(git, &canonical_root, &base_config_args)?; + Ok(Self { + git, + canonical_root, + base_config_args: base_config_args.into_boxed_slice(), + }) + } + + fn read_effective( + &self, + pattern: &str, + probe: &str, + ) -> io::Result> { + read_effective_config_with_fallback( + self.git, + &self.canonical_root, + &self.base_config_args, + pattern, + probe, + ) + } +} + +fn validate_base_config_args(args: &[String]) -> io::Result<()> { + let mut pairs = args.chunks_exact(2); + for pair in &mut pairs { + let Some((key, _value)) = pair[1].split_once('=') else { + return Err(invalid_base_config_args()); + }; + if pair[0] != "-c" || key.is_empty() { + return Err(invalid_base_config_args()); + } + } + if !pairs.remainder().is_empty() { + return Err(invalid_base_config_args()); + } + Ok(()) +} + +fn invalid_base_config_args() -> io::Error { + io::Error::new( + io::ErrorKind::InvalidInput, + "guarded Git base config must contain only ordered -c key=value pairs", + ) +} + +/// Operation-owned Git configuration capability. +/// +/// All operation children are rooted at the authorized repository, inherit +/// the exact frozen base invocation, receive fixed library safety scalars, and +/// retain any sealed filter override for the capability lifetime. +pub(crate) struct GuardedGitConfig<'git> { + sources: ValidatedConfigSources<'git>, + identity: Arc, + // Ordered, typed snapshots keep each sealed filter overlay alive through + // every later child. Downstream staging may attach a fresh Git-add policy + // without rebuilding or weakening the source authorization. + filters: Vec, +} + +struct CapabilityIdentity; + +#[derive(Clone, Copy)] +enum BoundSubcommand { + AddLiteralPathspecs, + Apply, + CheckAttr, + HashObject, + RevParse, +} + +impl BoundSubcommand { + fn as_str(self) -> &'static str { + match self { + Self::AddLiteralPathspecs => "add", + Self::Apply => "apply", + Self::CheckAttr => "check-attr", + Self::HashObject => "hash-object", + Self::RevParse => "rev-parse", + } + } + + fn append_to(self, command: &mut GitCommand) { + if matches!(self, Self::AddLiteralPathspecs) { + command.arg("--literal-pathspecs"); + } + command.arg(self.as_str()); + } +} + +/// A sealed filter-only include owned for the complete capability lifetime. +/// Its fields and include argument are intentionally inaccessible outside this +/// module; callers can neither forge one nor append it to another command. +pub(crate) struct SealedFilterConfigOverride { + owner: Arc, + include_arg: String, + _config_dir: tempfile::TempDir, +} + +impl SealedFilterConfigOverride { + fn append_to( + &self, + owner: &Arc, + command: &mut GitCommand, + ) -> io::Result<()> { + self.ensure_owner(owner)?; + command.args(["-c", &self.include_arg]); + Ok(()) + } + + fn append_rendered_args( + &self, + owner: &Arc, + args: &mut Vec, + ) -> io::Result<()> { + self.ensure_owner(owner)?; + args.push("-c".to_string()); + args.push(self.include_arg.clone()); + Ok(()) + } + + fn ensure_owner(&self, owner: &Arc) -> io::Result<()> { + if !Arc::ptr_eq(&self.owner, owner) { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "sealed Git filter override belongs to another operation", + )); + } + Ok(()) + } +} + +/// A command whose runner, root, config invocation, overlay lifetime, and +/// fixed subcommand are inseparably bound to one operation capability. +pub(crate) struct GuardedGitCommand<'operation, 'git> { + operation: &'operation GuardedGitConfig<'git>, + inner: GitCommand, +} + +impl GuardedGitCommand<'_, '_> { + pub(crate) fn arg(&mut self, arg: impl AsRef) -> &mut Self { + self.inner.arg(arg); + self + } + + pub(crate) fn args(&mut self, args: I) -> &mut Self + where + I: IntoIterator, + S: AsRef, + { + self.inner.args(args); + self + } + + pub(crate) fn disable_optional_locks(&mut self) -> &mut Self { + self.inner.env("GIT_OPTIONAL_LOCKS", "0"); + self + } + + pub(crate) fn stdin(&mut self, config: impl Into) -> &mut Self { + self.inner.stdin(config); + self + } + + pub(crate) fn output(self) -> io::Result { + self.operation.sources.git.output(self.inner) + } +} + +impl<'git> GuardedGitConfig<'git> { + pub(crate) fn authorize( + git: &'git GitRunner, + canonical_root: &Path, + base_config_args: Vec, + ) -> io::Result { + Ok(Self { + sources: ValidatedConfigSources::authorize(git, canonical_root, base_config_args)?, + identity: Arc::new(CapabilityIdentity), + filters: Vec::new(), + }) + } + + pub(crate) fn canonical_root(&self) -> &Path { + &self.sources.canonical_root + } + + pub(crate) fn apply_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::Apply) + } + + pub(crate) fn literal_add_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::AddLiteralPathspecs) + } + + pub(crate) fn rev_parse_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::RevParse) + } + + pub(crate) fn filter_attribute_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::CheckAttr) + } + + fn guarded_command( + &self, + subcommand: BoundSubcommand, + ) -> io::Result> { + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + command.args(&self.sources.base_config_args); + append_safe_scalar_overrides(&mut command); + for filter in &self.filters { + if let Some(neutralizer) = filter.neutralizer() { + neutralizer.append_to(&self.identity, &mut command)?; + } + } + subcommand.append_to(&mut command); + Ok(GuardedGitCommand { + operation: self, + inner: command, + }) + } + + pub(crate) fn command_for_sentinel_filter_probe<'operation>( + &'operation self, + neutralizer: &'operation SealedFilterConfigOverride, + driver: &str, + required: bool, + ) -> io::Result> { + if !matches!(driver, "set" | "unset" | "unspecified") { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "Git filter sentinel probe requested for a non-sentinel driver", + )); + } + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + command.args(&self.sources.base_config_args); + append_safe_scalar_overrides(&mut command); + for filter in &self.filters { + if let Some(attached) = filter.neutralizer() { + attached.append_to(&self.identity, &mut command)?; + } + } + neutralizer.append_to(&self.identity, &mut command)?; + command.args(["-c", &format!("filter.{driver}.required={required}")]); + BoundSubcommand::HashObject.append_to(&mut command); + Ok(GuardedGitCommand { + operation: self, + inner: command, + }) + } + + pub(crate) fn read_effective( + &self, + pattern: &str, + probe: &str, + ) -> io::Result> { + self.sources.read_effective(pattern, probe) + } + + fn ensure_owned_config_path(&self, path: &Path, description: &str) -> io::Result<()> { + self.sources + .git + .ensure_config_source_is_not_worktree_controlled(path, description) + } + + pub(crate) fn build_filter_override( + &self, + executable_drivers: &ExecutableFilterDrivers, + ) -> io::Result { + let config_dir = tempfile::tempdir()?; + let config_path = config_dir.path().join("filter-neutralization.gitconfig"); + self.ensure_owned_config_path(&config_path, "owned Git filter neutralization")?; + std::fs::write(&config_path, [])?; + for driver in executable_drivers.iter() { + for name in ["clean", "smudge", "process"] { + self.write_filter_override_value(&config_path, driver, name, "")?; + } + self.write_filter_override_value(&config_path, driver, "required", "false")?; + } + let config_path = config_path.to_str().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "non-UTF-8 filter guard path") + })?; + Ok(SealedFilterConfigOverride { + owner: Arc::clone(&self.identity), + include_arg: format!("include.path={config_path}"), + _config_dir: config_dir, + }) + } + + fn write_filter_override_value( + &self, + config_path: &Path, + driver: &str, + name: &str, + value: &str, + ) -> io::Result<()> { + debug_assert!(matches!(name, "clean" | "smudge" | "process" | "required")); + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + command + .args(&self.sources.base_config_args) + .args(["config", "--file"]) + .arg(config_path) + .args(["--add", &format!("filter.{driver}.{name}"), value]); + let output = self.sources.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(()) + } + + pub(crate) fn authorize_filter_paths(&mut self, paths: &[String]) -> io::Result<()> { + let filter = build_filter_policy_snapshot(self, paths)?; + self.filters.push(filter); + Ok(()) + } + + pub(crate) fn render_command_for_log(&self, args: &[String]) -> io::Result { + let mut parts = vec!["git".to_string()]; + parts.extend(self.sources.base_config_args.iter().cloned()); + parts.extend(safe_scalar_override_args()); + for filter in &self.filters { + if let Some(neutralizer) = filter.neutralizer() { + neutralizer.append_rendered_args(&self.identity, &mut parts)?; + } + } + parts.extend(args.iter().cloned()); + Ok(format!( + "(cd {} && {})", + quote_shell(&self.sources.canonical_root.display().to_string()), + parts + .into_iter() + .map(|part| quote_shell(&part)) + .collect::>() + .join(" ") + )) + } +} + +fn append_safe_scalar_overrides(command: &mut GitCommand) { + command.args(safe_scalar_override_args()); +} + +fn safe_scalar_override_args() -> [String; 4] { + [ + "-c".to_string(), + format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c".to_string(), + FsmonitorOverride::Disabled.git_config_arg().to_string(), + ] +} + +fn quote_shell(value: &str) -> String { + let simple = value + .chars() + .all(|character| character.is_ascii_alphanumeric() || "-_.:/@%+".contains(character)); + if simple { + value.to_string() + } else { + format!("'{}'", value.replace('\'', "'\\''")) + } +} + +#[cfg(test)] +thread_local! { + static CONFIG_SOURCE_AUTHORIZATION_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_config_source_authorization_count() { + CONFIG_SOURCE_AUTHORIZATION_COUNT.with(|count| count.set(0)); +} + +#[cfg(test)] +pub(crate) fn config_source_authorization_count() -> usize { + CONFIG_SOURCE_AUTHORIZATION_COUNT.with(std::cell::Cell::get) +} + +#[cfg(test)] +#[path = "guarded_config_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/guarded_config_tests.rs b/codex-rs/git-utils/src/guarded_config_tests.rs new file mode 100644 index 0000000000..70fdad0a79 --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config_tests.rs @@ -0,0 +1,116 @@ +use super::*; +use crate::safe_git::isolate_git_command_environment; +use pretty_assertions::assert_eq; + +fn run_git(cwd: &Path, args: &[&str]) { + let mut command = std::process::Command::new("git"); + isolate_git_command_environment(&mut command); + let output = command.current_dir(cwd).args(args).output().expect("Git"); + assert!( + output.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn attached_filter_snapshots_retain_multiple_owned_overrides() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + run_git( + repo.path(), + &["config", "filter.demo.clean", "git hash-object --stdin"], + ); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let mut config = + GuardedGitConfig::authorize(&git, repo.path(), Vec::new()).expect("authorized config"); + + config + .authorize_filter_paths(&["first.txt".to_string()]) + .expect("first filter snapshot"); + config + .authorize_filter_paths(&["second.txt".to_string()]) + .expect("second filter snapshot"); + + assert_eq!(config.filters.len(), 2); + assert!( + config + .filters + .iter() + .all(|filter| filter.neutralizer().is_some()) + ); +} + +#[test] +fn authorization_rejects_untyped_global_config_arguments() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + + for args in [ + vec!["--config-env=include.path=UNSNAPSHOTTED".to_string()], + vec!["--git-dir=elsewhere".to_string()], + vec!["-c".to_string(), "missing-value".to_string()], + vec!["-c".to_string()], + ] { + let error = match GuardedGitConfig::authorize(&git, repo.path(), args) { + Ok(_) => panic!("accepted untyped base config arguments"), + Err(error) => error, + }; + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); + } +} + +#[test] +fn authorization_rejects_another_runner_repository_and_nested_or_prefix_roots() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("repo"); + let nested = outer.join("nested"); + let prefix = fixture.path().join("repo-evil"); + std::fs::create_dir_all(&nested).expect("nested"); + std::fs::create_dir_all(&prefix).expect("prefix"); + run_git(&outer, &["init", "-q"]); + run_git(&nested, &["init", "-q"]); + run_git(&prefix, &["init", "-q"]); + let git = GitRunner::for_cwd_io(&outer).expect("outer runner"); + + for wrong_root in [&nested, &prefix] { + let error = match GuardedGitConfig::authorize(&git, wrong_root, Vec::new()) { + Ok(_) => panic!("accepted another repository root"), + Err(error) => error, + }; + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + } +} + +#[test] +fn sealed_filter_override_cannot_cross_operation_capabilities() { + let fixture = tempfile::tempdir().expect("fixture"); + let first_root = fixture.path().join("first"); + let second_root = fixture.path().join("second"); + std::fs::create_dir_all(&first_root).expect("first"); + std::fs::create_dir_all(&second_root).expect("second"); + run_git(&first_root, &["init", "-q"]); + run_git(&second_root, &["init", "-q"]); + run_git( + &first_root, + &["config", "filter.demo.clean", "git hash-object --stdin"], + ); + let first_git = GitRunner::for_cwd_io(&first_root).expect("first runner"); + let second_git = GitRunner::for_cwd_io(&second_root).expect("second runner"); + let mut first = + GuardedGitConfig::authorize(&first_git, &first_root, Vec::new()).expect("first capability"); + let second = GuardedGitConfig::authorize(&second_git, &second_root, Vec::new()) + .expect("second capability"); + first + .authorize_filter_paths(&["unselected.txt".to_string()]) + .expect("first filter policy"); + let sealed = first.filters[0].neutralizer().expect("first override"); + + let error = + match second.command_for_sentinel_filter_probe(sealed, "set", /*required*/ true) { + Ok(_) => panic!("accepted another operation's sealed override"), + Err(error) => error, + }; + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); +} diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index b63bddd518..0377521aef 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -6,8 +6,10 @@ mod errors; mod fsmonitor; mod git_command; mod git_config; +mod git_config_environment; mod git_config_sources; mod git_executable; +mod guarded_config; mod info; mod local_only; mod operations; diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index b42defd914..3bab04c74a 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -10,6 +10,18 @@ use crate::apply::write_temp_patch; use crate::git_command::GitRunner; use crate::git_config::path_is_within; use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::guarded_config::GuardedGitConfig; + +/// Extract effective patch paths through a bound operation configuration. +pub(crate) fn extract_effective_paths_from_patch_guarded( + config: &GuardedGitConfig<'_>, + patch_path: &Path, + revert: bool, +) -> io::Result> { + let forward_paths = git_apply_numstat_paths_guarded(config, patch_path, revert)?; + let reverse_paths = git_apply_numstat_paths_guarded(config, patch_path, !revert)?; + normalize_effective_patch_paths(forward_paths, reverse_paths) +} /// Extract paths with Git from a cwd whose config sources have already been /// authorized for `git_config_args`. @@ -26,6 +38,13 @@ pub(crate) fn extract_effective_paths_from_patch( // opposite orientation too so both endpoints are included in the result. let reverse_paths = git_apply_numstat_paths(git, authorized_cwd, patch_path, !revert, git_config_args)?; + normalize_effective_patch_paths(forward_paths, reverse_paths) +} + +fn normalize_effective_patch_paths( + forward_paths: Vec, + reverse_paths: Vec, +) -> io::Result> { if forward_paths.len() != reverse_paths.len() { return Err(io::Error::new( io::ErrorKind::InvalidData, @@ -46,6 +65,30 @@ pub(crate) fn extract_effective_paths_from_patch( .collect() } +fn git_apply_numstat_paths_guarded( + config: &GuardedGitConfig<'_>, + patch_path: &Path, + revert: bool, +) -> io::Result> { + let mut command = config.apply_command()?; + command.args(["--numstat", "-z"]); + if revert { + command.arg("-R"); + } + command.arg("--").arg(patch_path); + let output = command.output()?; + if !output.status.success() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!( + "failed to parse patch paths: {}", + String::from_utf8_lossy(&output.stderr).trim() + ), + )); + } + parse_numstat_paths(&output.stdout) +} + /// Best-effort extraction of the paths Git would apply. /// /// Security-sensitive callers must use the fallible internal extractor so an @@ -291,6 +334,37 @@ pub(crate) fn stage_effective_paths( Ok(()) } +/// Preserve the existing reverse staging behavior while constructing every +/// Git child from the operation-owned configuration capability. +pub(crate) fn stage_effective_paths_guarded( + config: &GuardedGitConfig<'_>, + paths: &[String], +) -> io::Result<()> { + let confined = confine_patch_paths_guarded(config, paths)?; + let mut existing = Vec::new(); + for path in confined.into_exact_leaves()? { + let joined = config.canonical_root().join(&path); + if let Ok(metadata) = std::fs::symlink_metadata(&joined) { + if leaf_is_traversable_directory(metadata.file_type()) { + return Err(containment_error( + "refusing to recursively stage a directory patch path", + )); + } + existing.push(path); + } + } + if existing.is_empty() { + return Ok(()); + } + let mut args = vec!["--".to_string()]; + args.extend(existing); + let mut command = config.literal_add_command()?; + command.args(&args); + let _output = command.output()?; + // Preserve the existing best-effort staging contract on non-zero status. + Ok(()) +} + #[cfg(not(windows))] fn leaf_is_traversable_directory(file_type: std::fs::FileType) -> bool { file_type.is_dir() @@ -379,6 +453,25 @@ pub(crate) fn confine_patch_paths( git: &GitRunner, git_root: &Path, paths: &[String], +) -> io::Result { + let canonical_root = std::fs::canonicalize(git_root)?; + let metadata_dirs = canonical_git_metadata_dirs(git, &canonical_root)?; + confine_patch_paths_with_metadata(&canonical_root, paths, &metadata_dirs) +} + +fn confine_patch_paths_guarded( + config: &GuardedGitConfig<'_>, + paths: &[String], +) -> io::Result { + let canonical_root = std::fs::canonicalize(config.canonical_root())?; + let metadata_dirs = canonical_git_metadata_dirs_guarded(config)?; + confine_patch_paths_with_metadata(&canonical_root, paths, &metadata_dirs) +} + +fn confine_patch_paths_with_metadata( + canonical_root: &Path, + paths: &[String], + metadata_dirs: &[PathBuf], ) -> io::Result { if paths.is_empty() { return Ok(ConfinedPatchPaths { @@ -401,8 +494,6 @@ pub(crate) fn confine_patch_paths( }); } - let canonical_root = std::fs::canonicalize(git_root)?; - let metadata_dirs = canonical_git_metadata_dirs(git, &canonical_root)?; let mut entries = Vec::with_capacity(paths.len()); let mut prefix_cache = std::collections::BTreeMap::new(); @@ -416,9 +507,9 @@ pub(crate) fn confine_patch_paths( ); let (existing_len, mut projected) = longest_existing_strict_prefix( - &canonical_root, + canonical_root, &components, - &metadata_dirs, + metadata_dirs, &mut prefix_cache, )?; projected.extend( @@ -546,6 +637,42 @@ fn canonical_git_metadata_dirs(git: &GitRunner, git_root: &Path) -> io::Result) -> io::Result> { + let queries = [ + ["rev-parse", "--absolute-git-dir"], + ["rev-parse", "--git-common-dir"], + ]; + let mut metadata_dirs = std::collections::BTreeSet::new(); + for args in queries { + let mut command = config.rev_parse_command()?; + command.args(&args[1..]); + let output = command.output()?; + if !output.status.success() { + return Err(io::Error::other(format!( + "failed to resolve Git repository metadata (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + let path = String::from_utf8_lossy(&output.stdout); + let path = path.trim_end_matches(['\r', '\n']); + if path.is_empty() { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "Git returned an empty repository metadata path", + )); + } + let path = PathBuf::from(path); + let absolute = if path.is_absolute() { + path + } else { + config.canonical_root().join(path) + }; + metadata_dirs.insert(std::fs::canonicalize(absolute)?); + } + Ok(metadata_dirs.into_iter().collect()) +} + fn insert_candidate_prefixes<'a>( components: impl IntoIterator, origin: ConfinedPathOrigin, diff --git a/codex-rs/git-utils/src/repository_authority/authority.rs b/codex-rs/git-utils/src/repository_authority/authority.rs index 05b792d357..cbdd194ce9 100644 --- a/codex-rs/git-utils/src/repository_authority/authority.rs +++ b/codex-rs/git-utils/src/repository_authority/authority.rs @@ -148,6 +148,23 @@ impl RepositoryAuthority { }) } + pub(crate) fn ensure_active_worktree_root(&self, root: &Path) -> io::Result<()> { + let canonical_root = std::fs::canonicalize(root)?; + let same_root = canonical_root == self.active_worktree_root + || same_file::is_same_file(&canonical_root, &self.active_worktree_root)?; + if !same_root { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "guarded Git repository root {} does not match runner repository {}", + canonical_root.display(), + self.active_worktree_root.display() + ), + )); + } + Ok(()) + } + #[cfg(test)] pub(crate) fn from_test_locations( roots: Vec, diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index e7904c001b..dcf9764099 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -3,20 +3,19 @@ use std::collections::BTreeSet; use std::io; use std::io::Seek; use std::io::Write; -use std::path::Path; use std::process::Command; use std::process::Stdio; -use crate::git_command::GitRunner; use crate::git_config::GitConfigEntry; -use crate::git_config::read_effective_config_with_fallback as read_effective_config_unchecked; -use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::guarded_config::GuardedGitConfig; +use crate::guarded_config::SealedFilterConfigOverride; #[path = "filter_sentinel.rs"] mod filter_sentinel; pub(crate) use filter_sentinel::SentinelFilterProbeBudget; pub(crate) use filter_sentinel::SentinelFilterProbeResolution; pub(crate) use filter_sentinel::classify_sentinel_filter_probes; +#[cfg(test)] pub(crate) use filter_sentinel::sentinel_filter_probe_config_args; pub(crate) const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = @@ -28,21 +27,32 @@ pub(crate) enum FilterAttributeValue { AmbiguousSentinel(String), } -pub(crate) struct GitFilterNeutralization { - git_config_args: Vec, - _config_dir: Option, - filter_config: BTreeMap, -} +/// Complete executable-filter inventory derived from one authorized effective +/// config read. The private field prevents a caller from minting an incomplete +/// inventory and treating its neutralizer as complete. +pub(crate) struct ExecutableFilterDrivers(BTreeSet); -impl GitFilterNeutralization { - pub(crate) fn git_config_args(&self) -> &[String] { - &self.git_config_args +impl ExecutableFilterDrivers { + pub(crate) fn iter(&self) -> impl Iterator { + self.0.iter() } - pub(crate) fn filter_value(&self, driver: &str, name: &str) -> Option<&str> { - self.filter_config - .get(&format!("filter.{driver}.{name}")) - .map(|entry| entry.value.as_str()) + fn is_empty(&self) -> bool { + self.0.is_empty() + } + + fn contains(&self, driver: &str) -> bool { + self.0.contains(driver) + } +} + +pub(crate) struct FilterPolicySnapshot { + neutralizer: Option, +} + +impl FilterPolicySnapshot { + pub(crate) fn neutralizer(&self) -> Option<&SealedFilterConfigOverride> { + self.neutralizer.as_ref() } } @@ -73,35 +83,22 @@ pub(crate) fn isolate_git_command_environment(command: &mut Command) { } } -pub(crate) fn ensure_no_selected_executable_git_filters( - git: &GitRunner, - cwd: &Path, +pub(crate) fn build_filter_policy_snapshot( + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], -) -> io::Result { - let entries = read_filter_config(git, cwd, git_config_args)?; +) -> io::Result { + let entries = read_filter_config(config)?; let executable_drivers = executable_filter_drivers(&entries)?; if executable_drivers.is_empty() { - return Ok(GitFilterNeutralization { - git_config_args: Vec::new(), - _config_dir: None, - filter_config: entries, - }); + return Ok(FilterPolicySnapshot { neutralizer: None }); } - let guard = executable_filter_guard(git, cwd, entries, &executable_drivers)?; + let neutralizer = config.build_filter_override(&executable_drivers)?; let paths = paths .iter() .map(|path| path.as_bytes().to_vec()) .collect::>(); - let attributes = read_filter_attributes( - git, - cwd, - &paths, - git_config_args, - &executable_drivers, - &guard, - )?; - if let Some((driver, path)) = selected_executable_filter(&guard.filter_config, &attributes)? { + let attributes = read_filter_attributes(config, &paths, &executable_drivers, &neutralizer)?; + if let Some((driver, path)) = selected_executable_filter(&entries, &attributes)? { return Err(io::Error::new( io::ErrorKind::Unsupported, format!( @@ -110,105 +107,22 @@ pub(crate) fn ensure_no_selected_executable_git_filters( ), )); } - Ok(guard) -} - -fn executable_filter_guard( - git: &GitRunner, - cwd: &Path, - filter_config: BTreeMap, - executable_drivers: &BTreeSet, -) -> io::Result { - 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, - }; - for driver in executable_drivers { - debug_assert!(["clean", "smudge", "process"].into_iter().any(|name| { - guard - .filter_value(driver, name) - .is_some_and(|value| !value.is_empty()) - })); - for command in ["clean", "smudge", "process"] { - guard.write_config_value(git, cwd, &config_path, driver, command, "")?; - } - 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_for_cwd(cwd)?; - command.args(["config", "--file"]).arg(config_path).args([ - "--add", - &format!("filter.{driver}.{name}"), - value, - ]); - 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(()) - } + Ok(FilterPolicySnapshot { + neutralizer: Some(neutralizer), + }) } fn read_filter_config( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], + config: &GuardedGitConfig<'_>, ) -> io::Result> { - read_effective_config_with_fallback( - git, - cwd, - git_config_args, - EXECUTABLE_FILTER_CONFIG_PATTERN, - "filter", - ) -} - -pub(crate) fn read_effective_config_with_fallback( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], - pattern: &str, - probe: &str, -) -> io::Result> { - ensure_no_worktree_config_sources(git, cwd, git_config_args)?; - read_effective_config_unchecked(git, cwd, git_config_args, pattern, probe) + config.read_effective(EXECUTABLE_FILTER_CONFIG_PATTERN, "filter") } fn read_filter_attributes( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, paths: &[Vec], - git_config_args: &[String], - executable_drivers: &BTreeSet, - neutralization: &GitFilterNeutralization, + executable_drivers: &ExecutableFilterDrivers, + neutralization: &SealedFilterConfigOverride, ) -> io::Result, String>> { if paths.is_empty() { return Ok(BTreeMap::new()); @@ -217,22 +131,12 @@ fn read_filter_attributes( write_nul_paths(&mut input, paths)?; input.rewind()?; - let mut command = git.command_for_cwd(cwd)?; + let mut command = config.filter_attribute_command()?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args([ - "-c", - &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c", - "core.fsmonitor=false", - "check-attr", - "--stdin", - "-z", - "filter", - ]) + .disable_optional_locks() + .args(["--stdin", "-z", "filter"]) .stdin(Stdio::from(input)); - let output = git.output(command)?; + let output = command.output()?; if !output.status.success() { return Err(io::Error::other(format!( "git filter attribute probe failed with status {}: {}", @@ -241,23 +145,14 @@ fn read_filter_attributes( ))); } let attributes = parse_filter_attributes(&output.stdout, paths)?; - resolve_filter_attribute_sentinels( - git, - cwd, - attributes, - git_config_args, - executable_drivers, - neutralization, - ) + resolve_filter_attribute_sentinels(config, attributes, executable_drivers, neutralization) } fn resolve_filter_attribute_sentinels( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, attributes: BTreeMap, FilterAttributeValue>, - git_config_args: &[String], - executable_drivers: &BTreeSet, - neutralization: &GitFilterNeutralization, + executable_drivers: &ExecutableFilterDrivers, + neutralization: &SealedFilterConfigOverride, ) -> io::Result, String>> { let mut resolved = BTreeMap::new(); let mut probe_budget = SentinelFilterProbeBudget::default(); @@ -269,11 +164,9 @@ fn resolve_filter_attribute_sentinels( FilterAttributeValue::AmbiguousSentinel(driver) => { if executable_drivers.contains(&driver) && sentinel_spelling_selects_filter_driver( - git, - cwd, + config, &path, &driver, - git_config_args, neutralization, &mut probe_budget, )? @@ -289,20 +182,16 @@ fn resolve_filter_attribute_sentinels( /// Disambiguate Git's sentinel spellings with required/optional probes. The /// shared guard blanks every known executable driver before either probe. fn sentinel_spelling_selects_filter_driver( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, path: &[u8], driver: &str, - git_config_args: &[String], - neutralization: &GitFilterNeutralization, + neutralization: &SealedFilterConfigOverride, probe_budget: &mut SentinelFilterProbeBudget, ) -> io::Result { let probe = SentinelSelectionProbe { - git, - cwd, + config, path, driver, - git_config_args, neutralization, }; let required = probe.run(/*required*/ true, probe_budget)?; @@ -328,12 +217,10 @@ fn sentinel_spelling_selects_filter_driver( } struct SentinelSelectionProbe<'a> { - git: &'a GitRunner, - cwd: &'a Path, + config: &'a GuardedGitConfig<'a>, path: &'a [u8], driver: &'a str, - git_config_args: &'a [String], - neutralization: &'a GitFilterNeutralization, + neutralization: &'a SealedFilterConfigOverride, } impl SentinelSelectionProbe<'_> { @@ -342,29 +229,26 @@ impl SentinelSelectionProbe<'_> { required: bool, probe_budget: &mut SentinelFilterProbeBudget, ) -> io::Result { - let probe_config_args = sentinel_filter_probe_config_args( - self.neutralization.git_config_args(), + if !matches!(self.driver, "set" | "unset" | "unspecified") { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "Git filter sentinel probe requested for a non-sentinel driver", + )); + } + let path = git_path_argument(self.path)?; + let mut command = self.config.command_for_sentinel_filter_probe( + self.neutralization, self.driver, required, )?; - let path = git_path_argument(self.path)?; - let mut command = self.git.command_for_cwd(self.cwd)?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(self.git_config_args) - .args([ - "-c", - &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c", - "core.fsmonitor=false", - ]) - .args(&probe_config_args) - .args(["hash-object", "--stdin"]) + .disable_optional_locks() + .arg("--stdin") .arg("--path") .arg(path) .stdin(Stdio::null()); probe_budget.ensure_probe_available()?; - let output = self.git.output(command)?; + let output = command.output()?; probe_budget.record_completed_probe(); Ok(output) } @@ -397,9 +281,9 @@ fn selected_executable_filter( Ok(None) } -pub(crate) fn executable_filter_drivers( +fn executable_filter_drivers( entries: &BTreeMap, -) -> io::Result> { +) -> io::Result { let mut executable_drivers = BTreeSet::new(); for entry in entries.values() { if entry.key.ends_with(".required") { @@ -410,7 +294,7 @@ pub(crate) fn executable_filter_drivers( executable_drivers.insert(driver); } } - Ok(executable_drivers) + Ok(ExecutableFilterDrivers(executable_drivers)) } fn filter_driver_name(key: &str) -> io::Result { diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs index b04cd9d6f5..2f45bad4d7 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -1,5 +1,7 @@ use super::*; +use crate::git_command::GitRunner; use crate::git_config::GitConfigScope; +use crate::guarded_config::GuardedGitConfig; use pretty_assertions::assert_eq; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -109,7 +111,7 @@ fn selected_filter_policy_allows_effective_empty_value() { } #[test] -fn filter_snapshot_retains_required_without_treating_it_as_executable() { +fn filter_config_retains_required_without_treating_it_as_executable() { let mut entries = filter_entries( GitConfigScope::Local, Path::new("config"), @@ -123,17 +125,16 @@ fn filter_snapshot_retains_required_without_treating_it_as_executable() { "true", )); assert_eq!( - executable_filter_drivers(&entries).expect("executable drivers"), + executable_filter_drivers(&entries) + .expect("executable drivers") + .0, BTreeSet::from(["demo".to_string()]) ); - let neutralization = GitFilterNeutralization { - git_config_args: Vec::new(), - _config_dir: None, - filter_config: entries, - }; assert_eq!( - neutralization.filter_value("demo", "required"), - Some("true") + entries + .get("filter.demo.required") + .map(|entry| entry.value.as_str()), + Some("true"), ); let required_only = filter_entries( @@ -263,9 +264,12 @@ fn sentinel_special_states_and_literal_driver_names_remain_distinct() { let root = repo.path(); configure_marker_filter(root, driver); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let mut config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); std::fs::write(root.join(".gitattributes"), special_rule).expect("write special attribute"); - ensure_no_selected_executable_git_filters(&git, root, &["file.txt".to_string()], &[]) + config + .authorize_filter_paths(&["file.txt".to_string()]) .expect("allow special attribute state"); assert!(!marker_filter_ran(root), "special {driver}"); @@ -274,8 +278,7 @@ fn sentinel_special_states_and_literal_driver_names_remain_distinct() { format!("file.txt filter={driver}\n"), ) .expect("write literal attribute"); - let result = - ensure_no_selected_executable_git_filters(&git, root, &["file.txt".to_string()], &[]); + let result = config.authorize_filter_paths(&["file.txt".to_string()]); let error = match result { Ok(_) => panic!("accepted literal sentinel-named driver {driver}"), Err(error) => error, @@ -296,9 +299,12 @@ fn sentinel_probe_neutralizes_every_known_driver_after_attribute_swap() { .expect("write initial attribute"); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - let entries = read_filter_config(&git, root, &[]).expect("filter config"); + let config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); + let entries = read_filter_config(&config).expect("filter config"); let executable_drivers = executable_filter_drivers(&entries).expect("executable drivers"); - let neutralization = executable_filter_guard(&git, root, entries, &executable_drivers) + let neutralization = config + .build_filter_override(&executable_drivers) .expect("filter neutralization"); let output = run_git_success(root, &["check-attr", "-z", "filter", "--", "file.txt"]); let attributes = parse_filter_attributes(&output.stdout, &[b"file.txt".to_vec()]) @@ -310,10 +316,8 @@ fn sentinel_probe_neutralizes_every_known_driver_after_attribute_swap() { ) .expect("swap attribute after snapshot"); let resolved = resolve_filter_attribute_sentinels( - &git, - root, + &config, attributes, - &[], &executable_drivers, &neutralization, ) @@ -329,9 +333,12 @@ fn high_cardinality_ordinary_sentinels_stop_at_hard_child_probe_budget() { let root = repo.path(); configure_marker_filter(root, "unspecified"); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - let entries = read_filter_config(&git, root, &[]).expect("filter config"); + let config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); + let entries = read_filter_config(&config).expect("filter config"); let executable_drivers = executable_filter_drivers(&entries).expect("executable drivers"); - let neutralization = executable_filter_guard(&git, root, entries, &executable_drivers) + let neutralization = config + .build_filter_override(&executable_drivers) .expect("filter neutralization"); let attributes = (0..=SentinelFilterProbeBudget::max_probes()) .map(|index| { @@ -343,10 +350,8 @@ fn high_cardinality_ordinary_sentinels_stop_at_hard_child_probe_budget() { .collect(); let error = resolve_filter_attribute_sentinels( - &git, - root, + &config, attributes, - &[], &executable_drivers, &neutralization, ) @@ -359,36 +364,31 @@ fn high_cardinality_ordinary_sentinels_stop_at_hard_child_probe_budget() { } #[test] -fn unrelated_sentinel_probe_failures_remain_generic_and_fail_closed() { +fn sentinel_probe_rejects_non_sentinel_driver_before_launch() { let repo = init_filter_repo(); let root = repo.path(); configure_marker_filter(root, "set"); std::fs::write(root.join(".gitattributes"), "file.txt filter\n") .expect("write special attribute"); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - let entries = read_filter_config(&git, root, &[]).expect("filter config"); + let config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); + let entries = read_filter_config(&config).expect("filter config"); let executable_drivers = executable_filter_drivers(&entries).expect("executable drivers"); - let neutralization = executable_filter_guard(&git, root, entries, &executable_drivers) + let neutralization = config + .build_filter_override(&executable_drivers) .expect("filter neutralization"); let mut budget = SentinelFilterProbeBudget::default(); - let malformed_config = ["-c".to_string(), "=".to_string()]; let error = sentinel_spelling_selects_filter_driver( - &git, - root, + &config, b"file.txt", - "set", - &malformed_config, + "ordinary", &neutralization, &mut budget, ) - .expect_err("malformed config must fail both probes"); - assert_eq!(error.kind(), io::ErrorKind::Other); - assert!( - error - .to_string() - .starts_with("git filter attribute selection probe failed with required status") - ); + .expect_err("reject non-sentinel probe"); + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); assert!(!marker_filter_ran(root)); } From f047aa7f31837b0ea65ec1ea914d32d2162b6796 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 07:22:29 -0700 Subject: [PATCH 7/8] test(git-utils): isolate filter policy cardinality tests --- codex-rs/git-utils/src/apply.rs | 16 +++++++++++ codex-rs/git-utils/src/patch_paths_tests.rs | 32 +++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index e79be238a3..932a39ad99 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -586,6 +586,22 @@ mod tests { #[test] fn apply_then_revert_success() { let _g = env_lock().lock().unwrap(); + if std::env::var_os("CODEX_GIT_UTILS_APPLY_ENV_CHILD").is_none() { + let config_dir = tempfile::tempdir().expect("config tempdir"); + let global_config = config_dir.path().join("global.gitconfig"); + let system_config = config_dir.path().join("system.gitconfig"); + std::fs::write(&global_config, "").expect("empty global config"); + std::fs::write(&system_config, "").expect("empty system config"); + run_isolated_test( + "apply::tests::apply_then_revert_success", + &[ + ("GIT_CONFIG_GLOBAL", global_config.as_os_str()), + ("GIT_CONFIG_SYSTEM", system_config.as_os_str()), + ], + ); + return; + } + let repo = init_repo(); let root = repo.path(); // Seed file and commit original content diff --git a/codex-rs/git-utils/src/patch_paths_tests.rs b/codex-rs/git-utils/src/patch_paths_tests.rs index 44a93f04b8..f0485ad8ca 100644 --- a/codex-rs/git-utils/src/patch_paths_tests.rs +++ b/codex-rs/git-utils/src/patch_paths_tests.rs @@ -717,6 +717,22 @@ fn stage_effective_paths( #[test] fn standalone_stage_paths_uses_one_source_authorization_and_one_filter_read() { + if std::env::var_os("CODEX_GIT_UTILS_PATH_ENV_CHILD").is_none() { + let config_dir = tempfile::tempdir().expect("config tempdir"); + let global_config = config_dir.path().join("global.gitconfig"); + let system_config = config_dir.path().join("system.gitconfig"); + std::fs::write(&global_config, "").expect("empty global config"); + std::fs::write(&system_config, "").expect("empty system config"); + run_isolated_test( + "patch_paths::tests::standalone_stage_paths_uses_one_source_authorization_and_one_filter_read", + &[ + ("GIT_CONFIG_GLOBAL", global_config.as_os_str()), + ("GIT_CONFIG_SYSTEM", system_config.as_os_str()), + ], + ); + return; + } + let repo = init_repo(); let root = repo.path(); std::fs::write(root.join("single.txt"), "single\n").expect("write staged file"); @@ -732,6 +748,22 @@ fn standalone_stage_paths_uses_one_source_authorization_and_one_filter_read() { #[test] fn composed_staging_freshly_observes_a_new_clean_driver_and_attribute() { + if std::env::var_os("CODEX_GIT_UTILS_PATH_ENV_CHILD").is_none() { + let config_dir = tempfile::tempdir().expect("config tempdir"); + let global_config = config_dir.path().join("global.gitconfig"); + let system_config = config_dir.path().join("system.gitconfig"); + std::fs::write(&global_config, "").expect("empty global config"); + std::fs::write(&system_config, "").expect("empty system config"); + run_isolated_test( + "patch_paths::tests::composed_staging_freshly_observes_a_new_clean_driver_and_attribute", + &[ + ("GIT_CONFIG_GLOBAL", global_config.as_os_str()), + ("GIT_CONFIG_SYSTEM", system_config.as_os_str()), + ], + ); + return; + } + let repo = init_repo(); let root = repo.path(); std::fs::write(root.join("file.txt"), "base\n").expect("write base file"); From 8cc31d9b53027d74e5cf07f0212326785e0aa847 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 07:45:18 -0700 Subject: [PATCH 8/8] test(git-utils): align merge guard tests with conventions --- .../src/guarded_config/merge_overlay.rs | 47 +------------------ .../src/guarded_config/merge_overlay_tests.rs | 44 +++++++++++++++++ codex-rs/git-utils/src/merge_driver_tests.rs | 1 + codex-rs/git-utils/src/reverse_apply_tests.rs | 3 +- 4 files changed, 48 insertions(+), 47 deletions(-) create mode 100644 codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs diff --git a/codex-rs/git-utils/src/guarded_config/merge_overlay.rs b/codex-rs/git-utils/src/guarded_config/merge_overlay.rs index af7454fe83..355b6e520b 100644 --- a/codex-rs/git-utils/src/guarded_config/merge_overlay.rs +++ b/codex-rs/git-utils/src/guarded_config/merge_overlay.rs @@ -329,48 +329,5 @@ pub(crate) fn merge_overlay_count() -> usize { } #[cfg(test)] -mod tests { - use super::*; - use crate::git_command::GitRunner; - use crate::safe_git::isolate_git_command_environment; - - fn run_git(cwd: &Path, args: &[&str]) { - let mut command = std::process::Command::new("git"); - isolate_git_command_environment(&mut command); - let output = command.current_dir(cwd).args(args).output().expect("Git"); - assert!( - output.status.success(), - "git {args:?}: {}", - String::from_utf8_lossy(&output.stderr) - ); - } - - #[test] - fn sealed_merge_override_rejects_another_operation_identity() { - let repo = tempfile::tempdir().expect("repo"); - run_git(repo.path(), &["init", "-q"]); - run_git(repo.path(), &["config", "merge.unused.driver", "false"]); - let first_git = GitRunner::for_cwd_io(repo.path()).expect("first runner"); - let second_git = GitRunner::for_cwd_io(repo.path()).expect("second runner"); - let mut first = - GuardedGitConfig::authorize(&first_git, repo.path(), Vec::new()).expect("first config"); - let mut second = GuardedGitConfig::authorize(&second_git, repo.path(), Vec::new()) - .expect("second config"); - first - .authorize_filter_paths(&["file.txt".to_string()]) - .expect("first apply snapshot"); - second - .authorize_filter_paths(&["file.txt".to_string()]) - .expect("second apply snapshot"); - - let snapshot = first.read_merge_config_snapshot().expect("merge snapshot"); - let neutralizer = first - .build_merge_override(&snapshot) - .expect("build merge override") - .expect("physical merge override"); - let error = second - .attach_merge_override(Some(neutralizer)) - .expect_err("cross-operation override must refuse"); - assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); - } -} +#[path = "merge_overlay_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs b/codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs new file mode 100644 index 0000000000..069e4ff8c5 --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs @@ -0,0 +1,44 @@ +use super::*; +use crate::git_command::GitRunner; +use crate::safe_git::isolate_git_command_environment; +use pretty_assertions::assert_eq; + +fn run_git(cwd: &Path, args: &[&str]) { + let mut command = std::process::Command::new("git"); + isolate_git_command_environment(&mut command); + let output = command.current_dir(cwd).args(args).output().expect("Git"); + assert!( + output.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn sealed_merge_override_rejects_another_operation_identity() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + run_git(repo.path(), &["config", "merge.unused.driver", "false"]); + let first_git = GitRunner::for_cwd_io(repo.path()).expect("first runner"); + let second_git = GitRunner::for_cwd_io(repo.path()).expect("second runner"); + let mut first = + GuardedGitConfig::authorize(&first_git, repo.path(), Vec::new()).expect("first config"); + let mut second = + GuardedGitConfig::authorize(&second_git, repo.path(), Vec::new()).expect("second config"); + first + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("first apply snapshot"); + second + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("second apply snapshot"); + + let snapshot = first.read_merge_config_snapshot().expect("merge snapshot"); + let neutralizer = first + .build_merge_override(&snapshot) + .expect("build merge override") + .expect("physical merge override"); + let error = second + .attach_merge_override(Some(neutralizer)) + .expect_err("cross-operation override must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); +} diff --git a/codex-rs/git-utils/src/merge_driver_tests.rs b/codex-rs/git-utils/src/merge_driver_tests.rs index 399adfdcbe..ba87579397 100644 --- a/codex-rs/git-utils/src/merge_driver_tests.rs +++ b/codex-rs/git-utils/src/merge_driver_tests.rs @@ -11,6 +11,7 @@ use crate::guarded_config::merge_config_read_count; use crate::guarded_config::merge_overlay_count; use crate::guarded_config::reset_config_source_authorization_count; use crate::guarded_config::reset_merge_policy_counts; +use pretty_assertions::assert_eq; use std::ffi::OsStr; use std::path::Path; diff --git a/codex-rs/git-utils/src/reverse_apply_tests.rs b/codex-rs/git-utils/src/reverse_apply_tests.rs index 09a0aceb66..c700a6b803 100644 --- a/codex-rs/git-utils/src/reverse_apply_tests.rs +++ b/codex-rs/git-utils/src/reverse_apply_tests.rs @@ -617,8 +617,7 @@ fn reverse_three_way_with_staging_candidate_uses_one_authorization() { let repo = init_repo(); let root = repo.path(); let base_contents = "01\n02\n03\n04\n05\n06\n07\n08\n09\n10\nbase\n12\n13\n14\n15\n"; - let theirs_contents = - "01\n02\n03\n04\n05\n06\n07\n08\n09\n10\ntheirs\n12\n13\n14\n15\n"; + let theirs_contents = "01\n02\n03\n04\n05\n06\n07\n08\n09\n10\ntheirs\n12\n13\n14\n15\n"; let independently_edited = "01\n02\n03\n04\n05\n06\n07\nEIGHT\n09\n10\ntheirs\n12\n13\n14\n15\n"; let expected = "01\n02\n03\n04\n05\n06\n07\nEIGHT\n09\n10\nbase\n12\n13\n14\n15\n";