diff --git a/codex-rs/core/src/commit_attribution.rs b/codex-rs/core/src/commit_attribution.rs index 440f043ac1..a30db353c6 100644 --- a/codex-rs/core/src/commit_attribution.rs +++ b/codex-rs/core/src/commit_attribution.rs @@ -22,30 +22,28 @@ const COMMIT_HOOK_NAMES: &[&str] = &[ pub(crate) fn configure_git_hooks_env_for_config( env: &mut HashMap, config: &Config, -) { +) -> Vec<(String, String)> { configure_git_hooks_env( env, config.codex_home.as_path(), config.commit_attribution.as_deref(), - ); + ) } pub(crate) fn configure_git_hooks_env( env: &mut HashMap, codex_home: &Path, config_attribution: Option<&str>, -) { - let Some(value) = resolve_attribution_value(config_attribution) else { - return; +) -> Vec<(String, String)> { + let Some((key, value)) = git_hooks_runtime_config(codex_home, config_attribution) else { + return Vec::new(); }; - let Ok(hooks_path) = ensure_codex_hook_scripts(codex_home, &value) else { - return; - }; - - set_git_runtime_config(env, "core.hooksPath", hooks_path.to_string_lossy().as_ref()); + set_git_runtime_config(env, &key, &value); + vec![(key, value)] } +#[cfg(test)] pub(crate) fn injected_git_config_env(env: &HashMap) -> Vec<(String, String)> { let mut pairs = env .iter() @@ -74,6 +72,18 @@ fn resolve_attribution_value(config_attribution: Option<&str>) -> Option } } +fn git_hooks_runtime_config( + codex_home: &Path, + config_attribution: Option<&str>, +) -> Option<(String, String)> { + let value = resolve_attribution_value(config_attribution)?; + let hooks_path = ensure_codex_hook_scripts(codex_home, &value).ok()?; + Some(( + "core.hooksPath".to_string(), + hooks_path.to_string_lossy().into_owned(), + )) +} + fn ensure_codex_hook_scripts(codex_home: &Path, value: &str) -> std::io::Result { let hooks_dir = codex_home.join("hooks").join("commit-attribution"); fs::create_dir_all(&hooks_dir)?; @@ -105,14 +115,48 @@ fn build_hook_script(hook_name: &str, value: &str) -> String { let escaped_value = value.replace('\'', "'\"'\"'"); let prepare_commit_msg_body = if hook_name == PREPARE_COMMIT_MSG_HOOK_NAME { format!( - "\nmsg_file=\"${{1:-}}\"\nif [[ -n \"$msg_file\" && -f \"$msg_file\" ]]; then\n git interpret-trailers \\\n --in-place \\\n --if-exists doNothing \\\n --if-missing add \\\n --trailer 'Co-authored-by={escaped_value}' \\\n \"$msg_file\" || true\nfi\n" + r#" +msg_file="${{1:-}}" +if [[ -n "$msg_file" && -f "$msg_file" ]]; then + git interpret-trailers \ + --in-place \ + --if-exists doNothing \ + --if-missing add \ + --trailer 'Co-authored-by={escaped_value}' \ + "$msg_file" || true +fi +"# ) } else { String::new() }; format!( - "#!/usr/bin/env bash\nset -euo pipefail\n\nunset GIT_CONFIG_COUNT\nwhile IFS='=' read -r name _; do\n case \"$name\" in\n GIT_CONFIG_KEY_*|GIT_CONFIG_VALUE_*) unset \"$name\" ;;\n esac\ndone < <(env)\n\nexisting_hooks_path=\"$(git config --path core.hooksPath 2>/dev/null || true)\"\nif [[ -z \"$existing_hooks_path\" ]]; then\n git_dir=\"$(git rev-parse --git-common-dir 2>/dev/null || git rev-parse --git-dir 2>/dev/null || true)\"\n if [[ -n \"$git_dir\" ]]; then\n existing_hooks_path=\"$git_dir/hooks\"\n fi\nfi\n\nif [[ -n \"$existing_hooks_path\" ]]; then\n existing_hook=\"$existing_hooks_path/{hook_name}\"\n if [[ -x \"$existing_hook\" && \"$existing_hook\" != \"$0\" ]]; then\n \"$existing_hook\" \"$@\"\n fi\nfi\n{prepare_commit_msg_body}" + r#"#!/usr/bin/env bash +set -euo pipefail + +unset GIT_CONFIG_COUNT +while IFS='=' read -r name _; do + case "$name" in + GIT_CONFIG_KEY_*|GIT_CONFIG_VALUE_*) unset "$name" ;; + esac +done < <(env) + +existing_hooks_path="$(git config --path core.hooksPath 2>/dev/null || true)" +if [[ -z "$existing_hooks_path" ]]; then + git_dir="$(git rev-parse --git-common-dir 2>/dev/null || git rev-parse --git-dir 2>/dev/null || true)" + if [[ -n "$git_dir" ]]; then + existing_hooks_path="$git_dir/hooks" + fi +fi + +if [[ -n "$existing_hooks_path" ]]; then + existing_hook="$existing_hooks_path/{hook_name}" + if [[ -x "$existing_hook" && "$existing_hook" != "$0" ]]; then + "$existing_hook" "$@" + fi +fi +{prepare_commit_msg_body}"# ) } diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index 9357f154fc..5c49547717 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -130,6 +130,7 @@ pub(crate) async fn execute_user_shell_command( session_shell.as_ref(), turn_context.cwd.as_path(), &turn_context.shell_environment_policy.r#set, + &[], ); let call_id = Uuid::new_v4().to_string(); diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index a9dd04a48e..1571281cdd 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -6,7 +6,6 @@ use std::sync::Arc; use crate::codex::TurnContext; use crate::commit_attribution::configure_git_hooks_env_for_config; -use crate::commit_attribution::injected_git_config_env; use crate::exec::ExecParams; use crate::exec_env::create_env; use crate::exec_policy::ExecApprovalRequest; @@ -326,9 +325,11 @@ impl ShellHandler { } = args; let mut exec_params = exec_params; - if session.features().enabled(Feature::CodexGitCommit) { - configure_git_hooks_env_for_config(&mut exec_params.env, turn.config.as_ref()); - } + let runtime_git_config_overrides = if session.features().enabled(Feature::CodexGitCommit) { + configure_git_hooks_env_for_config(&mut exec_params.env, turn.config.as_ref()) + } else { + Vec::new() + }; let dependency_env = session.dependency_env().await; if !dependency_env.is_empty() { exec_params.env.extend(dependency_env.clone()); @@ -340,10 +341,6 @@ impl ShellHandler { explicit_env_overrides.insert(key.clone(), value.clone()); } } - for (key, value) in injected_git_config_env(&exec_params.env) { - explicit_env_overrides.insert(key, value); - } - let exec_permission_approvals_enabled = session.features().enabled(Feature::ExecPermissionApprovals); let requested_additional_permissions = additional_permissions.clone(); @@ -443,6 +440,7 @@ impl ShellHandler { timeout_ms: exec_params.expiration.timeout_ms(), env: exec_params.env.clone(), explicit_env_overrides, + runtime_git_config_overrides, network: exec_params.network.clone(), sandbox_permissions: effective_additional_permissions.sandbox_permissions, additional_permissions: normalized_additional_permissions, diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index 8003819a84..7f6d029df0 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -70,6 +70,7 @@ pub(crate) fn maybe_wrap_shell_lc_with_snapshot( session_shell: &Shell, cwd: &Path, explicit_env_overrides: &HashMap, + runtime_git_config_overrides: &[(String, String)], ) -> Vec { if cfg!(windows) { return command.to_vec(); @@ -113,13 +114,14 @@ pub(crate) fn maybe_wrap_shell_lc_with_snapshot( .map(|arg| format!(" '{}'", shell_single_quote(arg))) .collect::(); let (override_captures, override_exports) = build_override_exports(explicit_env_overrides); - let rewritten_script = if override_exports.is_empty() { + let runtime_git_config_appends = build_runtime_git_config_appends(runtime_git_config_overrides); + let rewritten_script = if override_exports.is_empty() && runtime_git_config_appends.is_empty() { format!( "if . '{snapshot_path}' >/dev/null 2>&1; then :; fi\n\nexec '{original_shell}' -c '{original_script}'{trailing_args}" ) } else { format!( - "{override_captures}\n\nif . '{snapshot_path}' >/dev/null 2>&1; then :; fi\n\n{override_exports}\n\nexec '{original_shell}' -c '{original_script}'{trailing_args}" + "{override_captures}\n\nif . '{snapshot_path}' >/dev/null 2>&1; then :; fi\n\n{override_exports}{runtime_git_config_appends}\n\nexec '{original_shell}' -c '{original_script}'{trailing_args}" ) }; @@ -161,6 +163,29 @@ fn build_override_exports(explicit_env_overrides: &HashMap) -> ( (captures, restores) } +fn build_runtime_git_config_appends(runtime_git_config_overrides: &[(String, String)]) -> String { + if runtime_git_config_overrides.is_empty() { + return String::new(); + } + + let mut script = String::from( + "\ncase \"${GIT_CONFIG_COUNT:-}\" in\n ''|*[!0-9]*) __CODEX_GIT_CONFIG_INDEX=0 ;;\n *) __CODEX_GIT_CONFIG_INDEX=\"$GIT_CONFIG_COUNT\" ;;\nesac\n", + ); + + for (key, value) in runtime_git_config_overrides { + let key = shell_single_quote(key); + let value = shell_single_quote(value); + script.push_str(&format!( + "eval \"export GIT_CONFIG_KEY_${{__CODEX_GIT_CONFIG_INDEX}}='{key}'\"\n\ +eval \"export GIT_CONFIG_VALUE_${{__CODEX_GIT_CONFIG_INDEX}}='{value}'\"\n\ +__CODEX_GIT_CONFIG_INDEX=$((__CODEX_GIT_CONFIG_INDEX + 1))\n" + )); + } + + script.push_str("export GIT_CONFIG_COUNT=\"$__CODEX_GIT_CONFIG_INDEX\"\n"); + script +} + fn is_valid_shell_variable_name(name: &str) -> bool { let mut chars = name.chars(); let Some(first) = chars.next() else { diff --git a/codex-rs/core/src/tools/runtimes/mod_tests.rs b/codex-rs/core/src/tools/runtimes/mod_tests.rs index dbc341d1de..c1a36197c8 100644 --- a/codex-rs/core/src/tools/runtimes/mod_tests.rs +++ b/codex-rs/core/src/tools/runtimes/mod_tests.rs @@ -42,8 +42,13 @@ fn maybe_wrap_shell_lc_with_snapshot_bootstraps_in_user_shell() { "echo hello".to_string(), ]; - let rewritten = - maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path(), &HashMap::new()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + dir.path(), + &HashMap::new(), + &[], + ); assert_eq!(rewritten[0], "/bin/zsh"); assert_eq!(rewritten[1], "-c"); @@ -68,8 +73,13 @@ fn maybe_wrap_shell_lc_with_snapshot_escapes_single_quotes() { "echo 'hello'".to_string(), ]; - let rewritten = - maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path(), &HashMap::new()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + dir.path(), + &HashMap::new(), + &[], + ); assert!(rewritten[2].contains(r#"exec '/bin/bash' -c 'echo '"'"'hello'"'"''"#)); } @@ -91,8 +101,13 @@ fn maybe_wrap_shell_lc_with_snapshot_uses_bash_bootstrap_shell() { "echo hello".to_string(), ]; - let rewritten = - maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path(), &HashMap::new()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + dir.path(), + &HashMap::new(), + &[], + ); assert_eq!(rewritten[0], "/bin/bash"); assert_eq!(rewritten[1], "-c"); @@ -117,8 +132,13 @@ fn maybe_wrap_shell_lc_with_snapshot_uses_sh_bootstrap_shell() { "echo hello".to_string(), ]; - let rewritten = - maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path(), &HashMap::new()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + dir.path(), + &HashMap::new(), + &[], + ); assert_eq!(rewritten[0], "/bin/sh"); assert_eq!(rewritten[1], "-c"); @@ -145,8 +165,13 @@ fn maybe_wrap_shell_lc_with_snapshot_preserves_trailing_args() { "arg1".to_string(), ]; - let rewritten = - maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path(), &HashMap::new()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + dir.path(), + &HashMap::new(), + &[], + ); assert!( rewritten[2] @@ -171,8 +196,13 @@ fn maybe_wrap_shell_lc_with_snapshot_skips_when_cwd_mismatch() { "echo hello".to_string(), ]; - let rewritten = - maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, &command_cwd, &HashMap::new()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + &command_cwd, + &HashMap::new(), + &[], + ); assert_eq!(rewritten, command); } @@ -195,8 +225,13 @@ fn maybe_wrap_shell_lc_with_snapshot_accepts_dot_alias_cwd() { ]; let command_cwd = dir.path().join("."); - let rewritten = - maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, &command_cwd, &HashMap::new()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + &command_cwd, + &HashMap::new(), + &[], + ); assert_eq!(rewritten[0], "/bin/zsh"); assert_eq!(rewritten[1], "-c"); @@ -231,6 +266,7 @@ fn maybe_wrap_shell_lc_with_snapshot_restores_explicit_override_precedence() { &session_shell, dir.path(), &explicit_env_overrides, + &[], ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -265,8 +301,13 @@ fn maybe_wrap_shell_lc_with_snapshot_keeps_snapshot_path_without_override() { "-lc".to_string(), "printf '%s' \"$PATH\"".to_string(), ]; - let rewritten = - maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path(), &HashMap::new()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + dir.path(), + &HashMap::new(), + &[], + ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) .output() @@ -302,6 +343,7 @@ fn maybe_wrap_shell_lc_with_snapshot_applies_explicit_path_override() { &session_shell, dir.path(), &explicit_env_overrides, + &[], ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -342,6 +384,7 @@ fn maybe_wrap_shell_lc_with_snapshot_does_not_embed_override_values_in_argv() { &session_shell, dir.path(), &explicit_env_overrides, + &[], ); assert!(!rewritten[2].contains("super-secret-value")); @@ -386,6 +429,7 @@ fn maybe_wrap_shell_lc_with_snapshot_preserves_unset_override_variables() { &session_shell, dir.path(), &explicit_env_overrides, + &[], ); let output = Command::new(&rewritten[0]) @@ -396,3 +440,45 @@ fn maybe_wrap_shell_lc_with_snapshot_preserves_unset_override_variables() { assert!(output.status.success(), "command failed: {output:?}"); assert_eq!(String::from_utf8_lossy(&output.stdout), "unset"); } + +#[test] +fn maybe_wrap_shell_lc_with_snapshot_appends_runtime_git_config_after_snapshot() { + let dir = tempdir().expect("create temp dir"); + let snapshot_path = dir.path().join("snapshot.sh"); + std::fs::write( + &snapshot_path, + "# Snapshot file\nexport GIT_CONFIG_COUNT='1'\nexport GIT_CONFIG_KEY_0='user.name'\nexport GIT_CONFIG_VALUE_0='Pavel'\n", + ) + .expect("write snapshot"); + let session_shell = shell_with_snapshot( + ShellType::Bash, + "/bin/bash", + snapshot_path, + dir.path().to_path_buf(), + ); + let command = vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "printf '%s|%s|%s|%s|%s' \"$GIT_CONFIG_COUNT\" \"$GIT_CONFIG_KEY_0\" \"$GIT_CONFIG_VALUE_0\" \"$GIT_CONFIG_KEY_1\" \"$GIT_CONFIG_VALUE_1\"".to_string(), + ]; + let runtime_git_config_overrides = + vec![("core.hooksPath".to_string(), "/codex/hooks".to_string())]; + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + dir.path(), + &HashMap::new(), + &runtime_git_config_overrides, + ); + + let output = Command::new(&rewritten[0]) + .args(&rewritten[1..]) + .output() + .expect("run rewritten command"); + + assert!(output.status.success(), "command failed: {output:?}"); + assert_eq!( + String::from_utf8_lossy(&output.stdout), + "2|user.name|Pavel|core.hooksPath|/codex/hooks" + ); +} diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index daf8e82906..576d8842f7 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -48,6 +48,7 @@ pub struct ShellRequest { pub timeout_ms: Option, pub env: HashMap, pub explicit_env_overrides: HashMap, + pub runtime_git_config_overrides: Vec<(String, String)>, pub network: Option, pub sandbox_permissions: SandboxPermissions, pub additional_permissions: Option, @@ -225,6 +226,7 @@ impl ToolRuntime for ShellRuntime { session_shell.as_ref(), &req.cwd, &req.explicit_env_overrides, + &req.runtime_git_config_overrides, ); let command = if matches!(session_shell.shell_type, ShellType::PowerShell) && ctx.session.features().enabled(Feature::PowershellUtf8) diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 8d15c96c65..5d156dae5e 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -50,6 +50,7 @@ pub struct UnifiedExecRequest { pub cwd: PathBuf, pub env: HashMap, pub explicit_env_overrides: HashMap, + pub runtime_git_config_overrides: Vec<(String, String)>, pub network: Option, pub tty: bool, pub sandbox_permissions: SandboxPermissions, @@ -195,6 +196,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt session_shell.as_ref(), &req.cwd, &req.explicit_env_overrides, + &req.runtime_git_config_overrides, ); let command = if matches!(session_shell.shell_type, ShellType::PowerShell) && ctx.session.features().enabled(Feature::PowershellUtf8) diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 2eb18c2224..e3fa2c4562 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -14,7 +14,6 @@ use tokio::time::Instant; use tokio_util::sync::CancellationToken; use crate::commit_attribution::configure_git_hooks_env_for_config; -use crate::commit_attribution::injected_git_config_env; use crate::exec_env::create_env; use crate::exec_policy::ExecApprovalRequest; use crate::features::Feature; @@ -577,14 +576,14 @@ impl UnifiedExecProcessManager { &context.turn.shell_environment_policy, Some(context.session.conversation_id), ); - if context.turn.features.enabled(Feature::CodexGitCommit) { - configure_git_hooks_env_for_config(&mut env, context.turn.config.as_ref()); - } + let runtime_git_config_overrides = if context.turn.features.enabled(Feature::CodexGitCommit) + { + configure_git_hooks_env_for_config(&mut env, context.turn.config.as_ref()) + } else { + Vec::new() + }; let env = apply_unified_exec_env(env); - let mut explicit_env_overrides = context.turn.shell_environment_policy.r#set.clone(); - for (key, value) in injected_git_config_env(&env) { - explicit_env_overrides.insert(key, value); - } + let explicit_env_overrides = context.turn.shell_environment_policy.r#set.clone(); let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new(self, context.turn.tools_config.unified_exec_backend); @@ -610,6 +609,7 @@ impl UnifiedExecProcessManager { cwd, env, explicit_env_overrides, + runtime_git_config_overrides, network: request.network.clone(), tty: request.tty, sandbox_permissions: request.sandbox_permissions,