diff --git a/codex-rs/app-server/src/request_processors/command_exec_processor.rs b/codex-rs/app-server/src/request_processors/command_exec_processor.rs index 42e350a794..0c03ceede4 100644 --- a/codex-rs/app-server/src/request_processors/command_exec_processor.rs +++ b/codex-rs/app-server/src/request_processors/command_exec_processor.rs @@ -1,4 +1,5 @@ use super::*; +use codex_core::exec_env::inject_apply_patch_env; use codex_protocol::shell_environment::is_non_inheritable_env_var; #[derive(Clone)] @@ -165,6 +166,7 @@ impl CommandExecRequestProcessor { } } env.retain(|name, _| !is_non_inheritable_env_var(name)); + inject_apply_patch_env(&mut env, &self.config.features); let timeout_ms = match timeout_ms { Some(timeout_ms) => match u64::try_from(timeout_ms) { Ok(timeout_ms) => Some(timeout_ms), diff --git a/codex-rs/app-server/tests/suite/v2/command_exec.rs b/codex-rs/app-server/tests/suite/v2/command_exec.rs index 700af7049e..eec0797b9e 100644 --- a/codex-rs/app-server/tests/suite/v2/command_exec.rs +++ b/codex-rs/app-server/tests/suite/v2/command_exec.rs @@ -16,6 +16,7 @@ use codex_app_server_protocol::JSONRPCMessage; use codex_app_server_protocol::JSONRPCNotification; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::SandboxPolicy; +use codex_core::exec_env::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR; use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR; use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_READ_ONLY; use codex_protocol::shell_environment::OPENAI_FEDERATION_RULE_ID_ENV_VAR; @@ -198,6 +199,90 @@ async fn command_exec_env_overrides_merge_with_server_environment_and_support_un Ok(()) } +#[derive(Clone, Copy)] +enum CommandExecApplyPatchRollout { + Enabled, + Disabled, +} + +#[tokio::test] +async fn command_exec_apply_patch_preserves_line_endings_despite_client_override() -> Result<()> { + assert_command_exec_apply_patch_rollout( + CommandExecApplyPatchRollout::Enabled, + "0", + b"after\r\n", + ) + .await +} + +#[tokio::test] +async fn command_exec_apply_patch_normalizes_line_endings_despite_stale_overrides() -> Result<()> { + assert_command_exec_apply_patch_rollout(CommandExecApplyPatchRollout::Disabled, "1", b"after\n") + .await +} + +async fn assert_command_exec_apply_patch_rollout( + rollout: CommandExecApplyPatchRollout, + client_override: &str, + expected_contents: &[u8], +) -> Result<()> { + let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri(), "never")?; + + let feature_enabled = matches!(rollout, CommandExecApplyPatchRollout::Enabled); + insert_command_exec_config( + codex_home.path(), + &format!("[features]\napply_patch_preserve_line_endings = {feature_enabled}\n"), + )?; + + let workspace = TempDir::new()?; + let file_path = workspace.path().join("crlf.txt"); + std::fs::write(&file_path, b"before\r\n")?; + + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_auto_env() + .with_env_overrides(&[(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR, Some("1"))]) + .build_initialized_with_timeout(DEFAULT_READ_TIMEOUT) + .await?; + + let patch = "*** Begin Patch\n*** Update File: crlf.txt\n@@\n-before\n+after\n*** End Patch\n"; + let request_id = mcp + .send_command_exec_request(CommandExecParams { + command: vec!["apply_patch".to_string(), patch.to_string()], + process_id: None, + tty: false, + stream_stdin: false, + stream_stdout_stderr: false, + output_bytes_cap: None, + disable_output_cap: false, + disable_timeout: false, + timeout_ms: None, + cwd: Some(workspace.path().to_path_buf()), + env: Some(HashMap::from([( + CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(), + Some(client_override.to_string()), + )])), + size: None, + sandbox_policy: Some(SandboxPolicy::DangerFullAccess), + permission_profile: None, + }) + .await?; + + let response: CommandExecResponse = mcp.read_response(request_id).await?; + assert_eq!( + response, + CommandExecResponse { + exit_code: 0, + stdout: "Success. Updated the following files:\nM crlf.txt\n".to_string(), + stderr: String::new(), + } + ); + assert_eq!(std::fs::read(file_path)?, expected_contents); + Ok(()) +} + #[tokio::test] async fn command_exec_accepts_permission_profile() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; diff --git a/codex-rs/apply-patch/tests/suite/scenarios.rs b/codex-rs/apply-patch/tests/suite/scenarios.rs index b15e697664..ed8d6a37a4 100644 --- a/codex-rs/apply-patch/tests/suite/scenarios.rs +++ b/codex-rs/apply-patch/tests/suite/scenarios.rs @@ -1,5 +1,6 @@ +use anyhow::Context; use codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR; -use codex_utils_cargo_bin::repo_root; +use codex_utils_cargo_bin::find_resource; use pretty_assertions::assert_eq; use std::collections::BTreeMap; use std::fs; @@ -10,17 +11,18 @@ use tempfile::tempdir; #[test] fn test_apply_patch_scenarios() -> anyhow::Result<()> { - let scenarios_dir = repo_root()? - .join("codex-rs") - .join("apply-patch") - .join("tests") - .join("fixtures") - .join("scenarios"); - for scenario in fs::read_dir(scenarios_dir)? { + let scenarios_marker = find_resource!("tests/fixtures/scenarios/.gitattributes")?; + let scenarios_dir = scenarios_marker + .parent() + .context("scenario marker should have a parent directory")?; + for scenario in fs::read_dir(scenarios_dir) + .with_context(|| format!("failed to read {}", scenarios_dir.display()))? + { let scenario = scenario?; let path = scenario.path(); if path.is_dir() { - run_apply_patch_scenario(&path)?; + run_apply_patch_scenario(&path) + .with_context(|| format!("failed to run scenario {}", path.display()))?; } } Ok(()) @@ -38,7 +40,9 @@ fn run_apply_patch_scenario(dir: &Path) -> anyhow::Result<()> { } // Read the patch.txt file - let patch = fs::read_to_string(dir.join("patch.txt"))?; + let patch_path = dir.join("patch.txt"); + let patch = fs::read_to_string(&patch_path) + .with_context(|| format!("failed to read {}", patch_path.display()))?; // Run apply_patch in the temporary directory. We intentionally do not assert // on the exit status here; the scenarios are specified purely in terms of @@ -47,7 +51,8 @@ fn run_apply_patch_scenario(dir: &Path) -> anyhow::Result<()> { .arg(patch) .env(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR, "1") .current_dir(tmp.path()) - .output()?; + .output() + .with_context(|| format!("failed to run scenario {}", dir.display()))?; // Assert that the final state matches the expected state exactly let expected_dir = dir.join("expected"); diff --git a/codex-rs/apply-patch/tests/suite/tool.rs b/codex-rs/apply-patch/tests/suite/tool.rs index 7b994c0152..36d37efe1c 100644 --- a/codex-rs/apply-patch/tests/suite/tool.rs +++ b/codex-rs/apply-patch/tests/suite/tool.rs @@ -1,10 +1,5 @@ use assert_cmd::Command; -use codex_apply_patch::AppliedPatchDelta; -use codex_apply_patch::ApplyPatchFailure; -use codex_apply_patch::ApplyPatchFileUpdateMode; -use codex_apply_patch::apply_patch_with_mode; -use codex_exec_server::LOCAL_FS; -use codex_utils_path_uri::PathUri; +use codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR; use pretty_assertions::assert_eq; use std::fs; use std::path::Path; @@ -13,29 +8,12 @@ use tempfile::tempdir; fn run_apply_patch_in_dir(dir: &Path, patch: &str) -> anyhow::Result { let mut cmd = Command::new(codex_utils_cargo_bin::cargo_bin("apply_patch")?); + cmd.env(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR, "1"); cmd.current_dir(dir); Ok(cmd.arg(patch).assert()) } -async fn run_apply_patch_preserving_line_endings( - cwd: &PathUri, - patch: &str, - stdout: &mut Vec, - stderr: &mut Vec, -) -> Result { - apply_patch_with_mode( - patch, - ApplyPatchFileUpdateMode::PreserveLineEndings, - cwd, - stdout, - stderr, - LOCAL_FS.as_ref(), - /*sandbox*/ None, - ) - .await -} - -async fn assert_apply_patch_updates_file( +fn assert_apply_patch_updates_file( file_name: &str, original: &[u8], patch: &str, @@ -45,22 +23,19 @@ async fn assert_apply_patch_updates_file( let target_path = tmp.path().join(file_name); fs::write(&target_path, original)?; - let mut stdout = Vec::new(); - let mut stderr = Vec::new(); - let cwd = PathUri::from_host_native_path(tmp.path())?; - run_apply_patch_preserving_line_endings(&cwd, patch, &mut stdout, &mut stderr).await?; + run_apply_patch_in_dir(tmp.path(), patch)? + .success() + .stdout(format!( + "Success. Updated the following files:\nM {file_name}\n" + )); - assert_eq!( - String::from_utf8(stdout)?, - format!("Success. Updated the following files:\nM {file_name}\n") - ); - assert_eq!(stderr, Vec::::new()); assert_eq!(fs::read(target_path)?, expected); Ok(()) } fn apply_patch_command(dir: &Path) -> anyhow::Result { let mut cmd = Command::new(codex_utils_cargo_bin::cargo_bin("apply_patch")?); + cmd.env(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR, "1"); cmd.current_dir(dir); Ok(cmd) } @@ -112,30 +87,21 @@ fn test_apply_patch_cli_applies_multiple_chunks() -> anyhow::Result<()> { Ok(()) } -#[tokio::test] -async fn test_apply_patch_rejects_overlapping_end_of_file_chunks() -> anyhow::Result<()> { +#[test] +fn test_apply_patch_cli_rejects_overlapping_end_of_file_chunks() -> anyhow::Result<()> { let tmp = tempdir()?; let target_path = tmp.path().join("overlapping.txt"); + let expected_target_path = resolved_under(tmp.path(), "overlapping.txt")?; fs::write(&target_path, "one\n")?; let patch = "*** Begin Patch\n*** Update File: overlapping.txt\n@@\n-one\n+first\n@@\n-one\n+second\n*** End of File\n*** End Patch"; - let mut stdout = Vec::new(); - let mut stderr = Vec::new(); - let cwd = PathUri::from_host_native_path(tmp.path())?; - let error = run_apply_patch_preserving_line_endings(&cwd, patch, &mut stdout, &mut stderr) - .await - .expect_err("overlapping chunks should fail"); - - assert!(error.delta().is_empty()); - assert_eq!(stdout, Vec::::new()); - assert_eq!( - String::from_utf8(stderr)?, - format!( + run_apply_patch_in_dir(tmp.path(), patch)? + .failure() + .stderr(format!( "Failed to find expected lines in {}:\none\n", - target_path.display() - ) - ); + expected_target_path.display() + )); assert_eq!(fs::read_to_string(target_path)?, "one\n"); Ok(()) @@ -149,7 +115,11 @@ fn test_apply_patch_cli_allows_overlapping_eof_chunks_in_legacy_mode() -> anyhow let patch = "*** Begin Patch\n*** Update File: overlapping.txt\n@@\n-one\n+first\n@@\n-one\n+second\n*** End of File\n*** End Patch"; - run_apply_patch_in_dir(tmp.path(), patch)? + Command::new(codex_utils_cargo_bin::cargo_bin("apply_patch")?) + .env_remove(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR) + .arg(patch) + .current_dir(tmp.path()) + .assert() .success() .stdout("Success. Updated the following files:\nM overlapping.txt\n"); @@ -157,8 +127,8 @@ fn test_apply_patch_cli_allows_overlapping_eof_chunks_in_legacy_mode() -> anyhow Ok(()) } -#[tokio::test] -async fn test_apply_patch_preserves_crlf_from_target_file() -> anyhow::Result<()> { +#[test] +fn test_apply_patch_cli_preserves_crlf_from_target_file() -> anyhow::Result<()> { let patch = "*** Begin Patch\n*** Update File: crlf.txt\n@@\n-one\n+uno\n@@\n two\n+\n+between\n three\n*** End Patch"; assert_apply_patch_updates_file( @@ -167,11 +137,10 @@ async fn test_apply_patch_preserves_crlf_from_target_file() -> anyhow::Result<() patch, b"uno\r\ntwo\r\n\r\nbetween\r\nthree\r\n", ) - .await } -#[tokio::test] -async fn test_apply_patch_appends_after_trailing_blank_crlf_line() -> anyhow::Result<()> { +#[test] +fn test_apply_patch_cli_appends_after_trailing_blank_crlf_line() -> anyhow::Result<()> { let patch = "*** Begin Patch\n*** Update File: trailing_blank.txt\n@@\n+new\n*** End Patch"; assert_apply_patch_updates_file( @@ -180,24 +149,28 @@ async fn test_apply_patch_appends_after_trailing_blank_crlf_line() -> anyhow::Re patch, b"a\r\n\r\nnew\r\n", ) - .await } #[test] -fn test_apply_patch_cli_uses_legacy_line_handling_by_default() -> anyhow::Result<()> { +fn test_apply_patch_cli_uses_legacy_line_handling_without_rollout_env() -> anyhow::Result<()> { let tmp = tempdir()?; let target_path = tmp.path().join("crlf.txt"); fs::write(&target_path, b"one\r\n")?; let patch = "*** Begin Patch\n*** Update File: crlf.txt\n@@\n-one\n+uno\n*** End Patch"; - run_apply_patch_in_dir(tmp.path(), patch)?.success(); + Command::new(codex_utils_cargo_bin::cargo_bin("apply_patch")?) + .env_remove(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR) + .arg(patch) + .current_dir(tmp.path()) + .assert() + .success(); assert_eq!(fs::read(target_path)?, b"uno\n"); Ok(()) } -#[tokio::test] -async fn test_apply_patch_preserves_cr_from_target_file() -> anyhow::Result<()> { +#[test] +fn test_apply_patch_cli_preserves_cr_from_target_file() -> anyhow::Result<()> { let patch = "*** Begin Patch\n*** Update File: cr.txt\n@@\n-one\n+uno\n@@\n two\n+\n+between\n three\n*** End Patch"; assert_apply_patch_updates_file( @@ -206,28 +179,26 @@ async fn test_apply_patch_preserves_cr_from_target_file() -> anyhow::Result<()> patch, b"uno\rtwo\r\rbetween\rthree\r", ) - .await } -#[tokio::test] -async fn test_apply_patch_preserves_change_order_with_repeated_lines() -> anyhow::Result<()> { +#[test] +fn test_apply_patch_cli_preserves_change_order_with_repeated_lines() -> anyhow::Result<()> { let patch = "*** Begin Patch\n*** Update File: repeated.txt\n@@\n-a\n-b\n+b\n+b\n+a\n*** End Patch"; - assert_apply_patch_updates_file("repeated.txt", b"a\nb\n", patch, b"b\nb\na\n").await + assert_apply_patch_updates_file("repeated.txt", b"a\nb\n", patch, b"b\nb\na\n") } -#[tokio::test] -async fn test_apply_patch_preserves_repeated_context_line_ending() -> anyhow::Result<()> { +#[test] +fn test_apply_patch_cli_preserves_repeated_context_line_ending() -> anyhow::Result<()> { let patch = "*** Begin Patch\n*** Update File: repeated_context.txt\n@@\n-same\n same\n*** End Patch"; assert_apply_patch_updates_file("repeated_context.txt", b"same\r\nsame\n", patch, b"same\n") - .await } -#[tokio::test] -async fn test_apply_patch_preserves_untouched_mixed_line_endings() -> anyhow::Result<()> { +#[test] +fn test_apply_patch_cli_preserves_untouched_mixed_line_endings() -> anyhow::Result<()> { let patch = "*** Begin Patch\n*** Update File: mixed.txt\n@@\n one\n two\n-three\n+THREE\n four\n*** End Patch"; assert_apply_patch_updates_file( @@ -236,11 +207,10 @@ async fn test_apply_patch_preserves_untouched_mixed_line_endings() -> anyhow::Re patch, b"one\r\ntwo\rTHREE\r\nfour\r\n", ) - .await } -#[tokio::test] -async fn test_apply_patch_uses_crlf_for_new_trailing_newline() -> anyhow::Result<()> { +#[test] +fn test_apply_patch_cli_uses_crlf_for_new_trailing_newline() -> anyhow::Result<()> { let patch = "*** Begin Patch\n*** Update File: no_trailing_newline.txt\n@@\n-one\n+ONE\n*** End Patch"; @@ -250,7 +220,6 @@ async fn test_apply_patch_uses_crlf_for_new_trailing_newline() -> anyhow::Result patch, b"ONE\r\ntwo\r\n", ) - .await } #[test] diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index d7b52ff6ef..5d8c170fe6 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -434,6 +434,9 @@ "apply_patch_freeform": { "type": "boolean" }, + "apply_patch_preserve_line_endings": { + "type": "boolean" + }, "apply_patch_streaming_events": { "type": "boolean" }, @@ -5102,6 +5105,9 @@ "apply_patch_freeform": { "type": "boolean" }, + "apply_patch_preserve_line_endings": { + "type": "boolean" + }, "apply_patch_streaming_events": { "type": "boolean" }, diff --git a/codex-rs/core/src/exec_env.rs b/codex-rs/core/src/exec_env.rs index f33061698b..f4be869d42 100644 --- a/codex-rs/core/src/exec_env.rs +++ b/codex-rs/core/src/exec_env.rs @@ -1,3 +1,6 @@ +pub use codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR; +use codex_features::Feature; +use codex_features::Features; use codex_protocol::ThreadId; #[cfg(test)] use codex_protocol::config_types::EnvironmentVariablePattern; @@ -51,6 +54,22 @@ pub(crate) fn inject_permission_profile_env( } } +/// Carries the configured apply-patch line-ending rollout state into child +/// processes. +/// +/// Apply this after inherited or client-provided environment overrides so the +/// active feature configuration remains authoritative. The in-process +/// apply-patch path reads the feature directly. +pub fn inject_apply_patch_env(env: &mut HashMap, features: &Features) { + env.retain(|key, _| !key.eq_ignore_ascii_case(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR)); + if features.enabled(Feature::ApplyPatchPreserveLineEndings) { + env.insert( + CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(), + "1".to_string(), + ); + } +} + #[cfg(all(test, target_os = "windows"))] fn create_env_from_vars( vars: I, diff --git a/codex-rs/core/src/exec_env_tests.rs b/codex-rs/core/src/exec_env_tests.rs index 73c0944c14..b585232522 100644 --- a/codex-rs/core/src/exec_env_tests.rs +++ b/codex-rs/core/src/exec_env_tests.rs @@ -41,6 +41,28 @@ fn inject_permission_profile_env_removes_stale_value_without_active_profile() { assert_eq!(env.get(CODEX_PERMISSION_PROFILE_ENV_VAR), None); } +#[test] +fn inject_apply_patch_env_follows_preserve_line_endings_feature() { + let mut env = HashMap::from([( + CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_ascii_lowercase(), + "stale".to_string(), + )]); + let mut features = Features::with_defaults(); + + inject_apply_patch_env(&mut env, &features); + assert_eq!(env, HashMap::new()); + + features.enable(Feature::ApplyPatchPreserveLineEndings); + inject_apply_patch_env(&mut env, &features); + assert_eq!( + env, + HashMap::from([( + CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(), + "1".to_string(), + )]) + ); +} + #[cfg(target_os = "windows")] #[test] fn inject_permission_profile_env_replaces_differently_cased_windows_key() { diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index c8bdd5aa5b..293320fd7a 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -14,6 +14,7 @@ use crate::exec::ExecCapturePolicy; use crate::exec::StdoutStream; use crate::exec::execute_exec_request; use crate::exec_env::create_env; +use crate::exec_env::inject_apply_patch_env; use crate::sandboxing::ExecRequest; use crate::session::TurnInput; use crate::session::turn_context::TurnContext; @@ -159,6 +160,7 @@ pub(crate) async fn execute_user_shell_command( &turn_context.config.permissions.shell_environment_policy, Some(session.thread_id), ); + inject_apply_patch_env(&mut exec_env_map, &turn_context.config.features); if exec_env_map.contains_key(PROXY_ACTIVE_ENV_KEY) { strip_managed_proxy_env(&mut exec_env_map); } diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index f90d8d21db..a739e0e338 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -37,6 +37,7 @@ use crate::tools::runtimes::apply_patch::ApplyPatchRuntime; use crate::tools::sandboxing::ToolCtx; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; +use codex_apply_patch::ApplyPatchFileUpdateMode; use codex_apply_patch::Hunk; use codex_apply_patch::StreamingPatchParser; use codex_exec_server::ExecutorFileSystem; @@ -55,6 +56,19 @@ use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; const APPLY_PATCH_ARGUMENT_DIFF_BUFFER_INTERVAL: Duration = Duration::from_millis(500); + +fn apply_patch_file_update_mode(turn: &TurnContext) -> ApplyPatchFileUpdateMode { + if turn + .config + .features + .enabled(Feature::ApplyPatchPreserveLineEndings) + { + ApplyPatchFileUpdateMode::PreserveLineEndings + } else { + ApplyPatchFileUpdateMode::NormalizeToLf + } +} + /// Handles freeform `apply_patch` requests and routes verified patches to the /// selected environment filesystem. #[derive(Default)] @@ -387,9 +401,10 @@ impl ApplyPatchHandler { let fs = turn_environment.environment.get_filesystem(); let sandbox = turn .file_system_sandbox_context(/*additional_permissions*/ None, turn_environment); - match codex_apply_patch::verify_apply_patch_args( + match codex_apply_patch::verify_apply_patch_args_with_mode( args, turn_environment.cwd(), + apply_patch_file_update_mode(&turn), fs.as_ref(), Some(&sandbox), ) @@ -496,8 +511,14 @@ pub(crate) async fn intercept_apply_patch( let turn = &step_context.turn; let sandbox = turn.file_system_sandbox_context(/*additional_permissions*/ None, &turn_environment); - match codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd, fs, Some(&sandbox)) - .await + match codex_apply_patch::maybe_parse_apply_patch_verified_with_mode( + command, + cwd, + apply_patch_file_update_mode(turn), + fs, + Some(&sandbox), + ) + .await { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { let tool_ctx = ToolCtx { diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 71dc323c60..c964846bc1 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -44,6 +44,24 @@ async fn invocation_for_payload(payload: ToolPayload) -> ToolInvocation { } } +#[tokio::test] +async fn file_update_mode_follows_preserve_line_endings_feature() { + let (_, mut turn) = make_session_and_context().await; + assert_eq!( + apply_patch_file_update_mode(&turn), + codex_apply_patch::ApplyPatchFileUpdateMode::NormalizeToLf + ); + + Arc::make_mut(&mut turn.config) + .features + .enable(codex_features::Feature::ApplyPatchPreserveLineEndings) + .expect("feature should be enabled"); + assert_eq!( + apply_patch_file_update_mode(&turn), + codex_apply_patch::ApplyPatchFileUpdateMode::PreserveLineEndings + ); +} + #[tokio::test] async fn pre_tool_use_payload_uses_freeform_patch_input() { let patch = sample_patch(); diff --git a/codex-rs/core/src/tools/handlers/shell/shell_command.rs b/codex-rs/core/src/tools/handlers/shell/shell_command.rs index ae7dc8898e..0346455e5b 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_command.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_command.rs @@ -6,6 +6,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use crate::exec::ExecCapturePolicy; use crate::exec::ExecParams; use crate::exec_env::create_env; +use crate::exec_env::inject_apply_patch_env; use crate::exec_env::inject_permission_profile_env; use crate::function_tool::FunctionCallError; use crate::maybe_emit_implicit_skill_invocation; @@ -105,6 +106,7 @@ impl ShellCommandHandler { &turn_context.config.permissions.shell_environment_policy, Some(session.thread_id), ); + inject_apply_patch_env(&mut env, &turn_context.config.features); let active_permission_profile = turn_environment.active_permission_profile(); inject_permission_profile_env(&mut env, active_permission_profile.as_ref()); let sandbox_permissions = resolve_sandbox_permissions( diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 4ea6e29046..473cc650d6 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -164,8 +164,9 @@ impl ToolRuntime for ApplyPatchRunti let sandbox = Self::file_system_sandbox_context_for_attempt(req, attempt); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - let result = codex_apply_patch::apply_patch( + let result = codex_apply_patch::apply_patch_with_mode( &req.action.patch, + req.action.update_file_mode(), &req.action.cwd, &mut stdout, &mut stderr, diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index 9fc740fca4..e7c109a5b7 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -10,6 +10,7 @@ use crate::sandboxing::SandboxPermissions; use crate::shell::Shell; use crate::shell::ShellType; use crate::tools::sandboxing::ToolError; +use codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR; #[cfg(unix)] use codex_install_context::InstallContext; #[cfg(target_os = "macos")] @@ -267,14 +268,23 @@ pub(crate) fn maybe_wrap_shell_lc_with_snapshot( .map(|arg| format!(" '{}'", shell_single_quote(arg))) .collect::(); let mut override_env = explicit_env_overrides.clone(); - for key in [CODEX_THREAD_ID_ENV_VAR, CODEX_PERMISSION_PROFILE_ENV_VAR] { + for key in [ + CODEX_THREAD_ID_ENV_VAR, + CODEX_PERMISSION_PROFILE_ENV_VAR, + CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR, + ] { if let Some(value) = env.get(key) { override_env.insert(key.to_string(), value.clone()); } } - // Do not let a snapshot resurrect a stale profile when no named profile is active. - let (override_captures, override_exports) = - build_override_exports(&override_env, &[CODEX_PERMISSION_PROFILE_ENV_VAR]); + // Do not let a snapshot resurrect stale runtime state when it is inactive. + let (override_captures, override_exports) = build_override_exports( + &override_env, + &[ + CODEX_PERMISSION_PROFILE_ENV_VAR, + CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR, + ], + ); let (proxy_captures, proxy_exports) = build_proxy_env_exports(); let runtime_path_prepend_exports = runtime_path_prepends.shell_exports_after_snapshot(explicit_env_overrides); diff --git a/codex-rs/core/src/tools/runtimes/mod_tests.rs b/codex-rs/core/src/tools/runtimes/mod_tests.rs index 3b1fb1b9db..fb710fd0c2 100644 --- a/codex-rs/core/src/tools/runtimes/mod_tests.rs +++ b/codex-rs/core/src/tools/runtimes/mod_tests.rs @@ -607,6 +607,61 @@ fn maybe_wrap_shell_lc_with_snapshot_unsets_absent_permission_profile() { assert_eq!(output.stdout, b""); } +#[test] +fn maybe_wrap_shell_lc_with_snapshot_restores_apply_patch_rollout_state() { + let dir = tempdir().expect("create temp dir"); + let snapshot_path = dir.path().join("snapshot.sh"); + std::fs::write( + &snapshot_path, + "# Snapshot file\nexport CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS='stale'\n", + ) + .expect("write snapshot"); + let (session_shell, shell_snapshot) = + shell_with_snapshot(ShellType::Bash, "/bin/bash", snapshot_path.abs()); + let command = vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "printenv CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS".to_string(), + ]; + let env = HashMap::from([( + CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(), + "1".to_string(), + )]); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + Some(&shell_snapshot), + &HashMap::new(), + &env, + &RuntimePathPrepends::default(), + ); + let output = Command::new(&rewritten[0]) + .args(&rewritten[1..]) + .env(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR, "1") + .output() + .expect("run rewritten command"); + + assert!(output.status.success(), "command failed: {output:?}"); + assert_eq!(String::from_utf8_lossy(&output.stdout), "1\n"); + + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + Some(&shell_snapshot), + &HashMap::new(), + &HashMap::new(), + &RuntimePathPrepends::default(), + ); + let output = Command::new(&rewritten[0]) + .args(&rewritten[1..]) + .env_remove(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR) + .output() + .expect("run rewritten command"); + + assert_eq!(output.status.code(), Some(1)); + assert_eq!(output.stdout, b""); +} + #[test] fn maybe_wrap_shell_lc_with_snapshot_restores_proxy_env_from_process_env() { let dir = tempdir().expect("create temp dir"); diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 4a4c1ba1c1..6a9afc78c1 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -16,6 +16,7 @@ use crate::codex_thread::BackgroundTerminalInfo; use crate::exec_env::CODEX_PERMISSION_PROFILE_ENV_VAR; use crate::exec_env::CODEX_THREAD_ID_ENV_VAR; use crate::exec_env::create_env; +use crate::exec_env::inject_apply_patch_env; use crate::exec_env::inject_permission_profile_env; use crate::exec_policy::ExecApprovalRequest; use crate::sandboxing::ExecOptions; @@ -122,10 +123,18 @@ fn exec_env_policy_from_shell_policy( .iter() .map(std::string::ToString::to_string) .collect::>(); - exclude.push(CODEX_PERMISSION_PROFILE_ENV_VAR.to_string()); + exclude.extend([ + CODEX_PERMISSION_PROFILE_ENV_VAR.to_string(), + codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(), + ]); let mut r#set = policy.r#set.clone(); r#set.retain(|key, _| { - !key.eq_ignore_ascii_case(CODEX_PERMISSION_PROFILE_ENV_VAR) + ![ + CODEX_PERMISSION_PROFILE_ENV_VAR, + codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR, + ] + .iter() + .any(|runtime_key| key.eq_ignore_ascii_case(runtime_key)) && !is_non_inheritable_env_var(key) }); codex_exec_server::ExecEnvPolicy { @@ -149,8 +158,11 @@ fn env_overlay_for_exec_server( .iter() .filter(|(key, value)| { !is_non_inheritable_env_var(key) - && (key.as_str() == CODEX_PERMISSION_PROFILE_ENV_VAR - || local_policy_env.get(*key) != Some(*value)) + && (matches!( + key.as_str(), + CODEX_PERMISSION_PROFILE_ENV_VAR + | codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR + ) || local_policy_env.get(*key) != Some(*value)) }) .map(|(key, value)| (key.clone(), value.clone())) .collect() @@ -1148,6 +1160,7 @@ impl UnifiedExecProcessManager { CODEX_THREAD_ID_ENV_VAR.to_string(), context.session.thread_id.to_string(), ); + inject_apply_patch_env(&mut env, &turn.config.features); let active_permission_profile = request.turn_environment.active_permission_profile(); inject_permission_profile_env(&mut env, active_permission_profile.as_ref()); let env = apply_unified_exec_env(env); diff --git a/codex-rs/core/src/unified_exec/process_manager_tests.rs b/codex-rs/core/src/unified_exec/process_manager_tests.rs index 613b2f06d9..31ac76bb65 100644 --- a/codex-rs/core/src/unified_exec/process_manager_tests.rs +++ b/codex-rs/core/src/unified_exec/process_manager_tests.rs @@ -47,6 +47,10 @@ fn env_overlay_for_exec_server_keeps_runtime_changes_only() { CODEX_PERMISSION_PROFILE_ENV_VAR.to_string(), "current-profile".to_string(), ), + ( + codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(), + "1".to_string(), + ), ]); let request_env = HashMap::from([ ("HOME".to_string(), "/client-home".to_string()), @@ -58,6 +62,10 @@ fn env_overlay_for_exec_server_keeps_runtime_changes_only() { CODEX_PERMISSION_PROFILE_ENV_VAR.to_string(), "current-profile".to_string(), ), + ( + codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(), + "1".to_string(), + ), ( "CODEX_SANDBOX_NETWORK_DISABLED".to_string(), "1".to_string(), @@ -73,6 +81,10 @@ fn env_overlay_for_exec_server_keeps_runtime_changes_only() { CODEX_PERMISSION_PROFILE_ENV_VAR.to_string(), "current-profile".to_string(), ), + ( + codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(), + "1".to_string(), + ), ( "CODEX_SANDBOX_NETWORK_DISABLED".to_string(), "1".to_string() @@ -93,6 +105,10 @@ fn exec_env_policy_excludes_non_inheritable_and_runtime_variables() { "openai_identity_token_file".to_string(), "/run/identity-token".to_string(), ), + ( + "codex_apply_patch_preserve_line_endings".to_string(), + "1".to_string(), + ), ("KEEP".to_string(), "value".to_string()), ]), ..Default::default() @@ -103,7 +119,10 @@ fn exec_env_policy_excludes_non_inheritable_and_runtime_variables() { codex_exec_server::ExecEnvPolicy { inherit: policy.inherit, ignore_default_excludes: policy.ignore_default_excludes, - exclude: vec![CODEX_PERMISSION_PROFILE_ENV_VAR.to_string()], + exclude: vec![ + CODEX_PERMISSION_PROFILE_ENV_VAR.to_string(), + codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR.to_string(), + ], r#set: HashMap::from([("KEEP".to_string(), "value".to_string())]), include_only: Vec::new(), } diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index 0c4b35f1f0..3bb5f4e4a1 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -257,6 +257,112 @@ fn apply_patch_responses( ] } +async fn assert_apply_patch_crlf_update( + configure: impl FnOnce(TestCodexBuilder) -> TestCodexBuilder, + model_output: CrLfApplyPatchModelOutput, + expected: &str, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + let harness = apply_patch_harness_with(configure).await?; + let call_id = "apply-patch-crlf-rollout"; + let file_name = "crlf.txt"; + harness.write_file(file_name, "before\r\n").await?; + let patch = format!( + "*** Begin Patch\n*** Update File: {file_name}\n@@\n-before\n+after\n*** End Patch\n" + ); + match model_output { + CrLfApplyPatchModelOutput::CustomTool => { + mount_apply_patch(&harness, call_id, &patch, "apply_patch done").await; + } + CrLfApplyPatchModelOutput::ShellCommandViaHeredoc => { + mount_apply_patch_model_output( + &harness, + call_id, + &patch, + "apply_patch done", + ApplyPatchModelOutput::ShellCommandViaHeredoc, + ) + .await; + } + } + + harness + .test() + .submit_turn_with_permission_profile( + "update the CRLF file with apply_patch", + PermissionProfile::Disabled, + ) + .await?; + + assert_eq!(harness.read_file_text(file_name).await?, expected); + Ok(()) +} + +#[derive(Clone, Copy)] +enum CrLfApplyPatchModelOutput { + CustomTool, + ShellCommandViaHeredoc, +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn apply_patch_normalizes_crlf_without_preserve_line_endings_feature() -> Result<()> { + assert_apply_patch_crlf_update( + |builder| builder, + CrLfApplyPatchModelOutput::CustomTool, + "after\n", + ) + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn apply_patch_preserves_crlf_with_preserve_line_endings_feature() -> Result<()> { + assert_apply_patch_crlf_update( + |builder| { + builder.with_config(|config| { + config + .features + .enable(Feature::ApplyPatchPreserveLineEndings) + .expect("feature should be enabled"); + }) + }, + CrLfApplyPatchModelOutput::CustomTool, + "after\r\n", + ) + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn apply_patch_shell_heredoc_normalizes_crlf_without_preserve_line_endings_feature() +-> Result<()> { + skip_if_wine_exec!(Ok(()), "uses a POSIX shell heredoc"); + assert_apply_patch_crlf_update( + |builder| builder, + CrLfApplyPatchModelOutput::ShellCommandViaHeredoc, + "after\n", + ) + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn apply_patch_shell_heredoc_preserves_crlf_with_preserve_line_endings_feature() -> Result<()> +{ + skip_if_wine_exec!(Ok(()), "uses a POSIX shell heredoc"); + assert_apply_patch_crlf_update( + |builder| { + builder.with_config(|config| { + config + .features + .enable(Feature::ApplyPatchPreserveLineEndings) + .expect("feature should be enabled"); + }) + }, + CrLfApplyPatchModelOutput::ShellCommandViaHeredoc, + "after\r\n", + ) + .await +} + #[cfg(target_os = "linux")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn apply_patch_cli_uses_codex_self_exe_with_linux_sandbox_helper_alias() -> Result<()> { diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 7f08f62643..8daf9e10d1 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -123,6 +123,8 @@ pub enum Feature { TerminalVisualizationInstructions, /// Stream structured progress while apply_patch input is being generated. ApplyPatchStreamingEvents, + /// Preserve existing line endings when apply_patch updates files. + ApplyPatchPreserveLineEndings, /// Allow exec tools to request additional permissions while staying sandboxed. ExecPermissionApprovals, /// Expose the built-in request_permissions tool. @@ -1045,6 +1047,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::UnderDevelopment, default_enabled: false, }, + FeatureSpec { + id: Feature::ApplyPatchPreserveLineEndings, + key: "apply_patch_preserve_line_endings", + stage: Stage::UnderDevelopment, + default_enabled: false, + }, FeatureSpec { id: Feature::ExecPermissionApprovals, key: "exec_permission_approvals",