Skip to content

Commit 5b25915

Browse files
fix(apply_patch) tests for shell_command (#7307)
## Summary Adds test coverage for invocations of apply_patch via shell_command with heredoc, to validate behavior. ## Testing - [x] These are tests
1 parent c0564ed commit 5b25915

File tree

3 files changed

+124
-13
lines changed

3 files changed

+124
-13
lines changed

codex-rs/core/tests/common/responses.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,9 @@ pub fn ev_apply_patch_call(
431431
ApplyPatchModelOutput::ShellViaHeredoc => {
432432
ev_apply_patch_shell_call_via_heredoc(call_id, patch)
433433
}
434+
ApplyPatchModelOutput::ShellCommandViaHeredoc => {
435+
ev_apply_patch_shell_command_call_via_heredoc(call_id, patch)
436+
}
434437
}
435438
}
436439

@@ -492,6 +495,13 @@ pub fn ev_apply_patch_shell_call_via_heredoc(call_id: &str, patch: &str) -> Valu
492495
ev_function_call(call_id, "shell", &arguments)
493496
}
494497

498+
pub fn ev_apply_patch_shell_command_call_via_heredoc(call_id: &str, patch: &str) -> Value {
499+
let args = serde_json::json!({ "command": format!("apply_patch <<'EOF'\n{patch}\nEOF\n") });
500+
let arguments = serde_json::to_string(&args).expect("serialize apply_patch arguments");
501+
502+
ev_function_call(call_id, "shell_command", &arguments)
503+
}
504+
495505
pub fn sse_failed(id: &str, code: &str, message: &str) -> String {
496506
sse(vec![serde_json::json!({
497507
"type": "response.failed",

codex-rs/core/tests/common/test_codex.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ pub enum ApplyPatchModelOutput {
3636
Function,
3737
Shell,
3838
ShellViaHeredoc,
39+
ShellCommandViaHeredoc,
3940
}
4041

4142
/// A collection of different ways the model can output an apply_patch call
@@ -312,7 +313,10 @@ impl TestCodexHarness {
312313
ApplyPatchModelOutput::Freeform => self.custom_tool_call_output(call_id).await,
313314
ApplyPatchModelOutput::Function
314315
| ApplyPatchModelOutput::Shell
315-
| ApplyPatchModelOutput::ShellViaHeredoc => self.function_call_stdout(call_id).await,
316+
| ApplyPatchModelOutput::ShellViaHeredoc
317+
| ApplyPatchModelOutput::ShellCommandViaHeredoc => {
318+
self.function_call_stdout(call_id).await
319+
}
316320
}
317321
}
318322
}

codex-rs/core/tests/suite/apply_patch_cli.rs

Lines changed: 109 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
use anyhow::Result;
44
use core_test_support::responses::ev_apply_patch_call;
5+
use core_test_support::responses::ev_shell_command_call;
56
use core_test_support::test_codex::ApplyPatchModelOutput;
67
use pretty_assertions::assert_eq;
78
use std::fs;
@@ -127,6 +128,7 @@ D delete.txt
127128
#[test_case(ApplyPatchModelOutput::Function)]
128129
#[test_case(ApplyPatchModelOutput::Shell)]
129130
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
131+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
130132
async fn apply_patch_cli_multiple_chunks(model_output: ApplyPatchModelOutput) -> Result<()> {
131133
skip_if_no_network!(Ok(()));
132134

@@ -153,6 +155,7 @@ async fn apply_patch_cli_multiple_chunks(model_output: ApplyPatchModelOutput) ->
153155
#[test_case(ApplyPatchModelOutput::Function)]
154156
#[test_case(ApplyPatchModelOutput::Shell)]
155157
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
158+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
156159
async fn apply_patch_cli_moves_file_to_new_directory(
157160
model_output: ApplyPatchModelOutput,
158161
) -> Result<()> {
@@ -181,6 +184,7 @@ async fn apply_patch_cli_moves_file_to_new_directory(
181184
#[test_case(ApplyPatchModelOutput::Function)]
182185
#[test_case(ApplyPatchModelOutput::Shell)]
183186
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
187+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
184188
async fn apply_patch_cli_updates_file_appends_trailing_newline(
185189
model_output: ApplyPatchModelOutput,
186190
) -> Result<()> {
@@ -208,6 +212,7 @@ async fn apply_patch_cli_updates_file_appends_trailing_newline(
208212
#[test_case(ApplyPatchModelOutput::Function)]
209213
#[test_case(ApplyPatchModelOutput::Shell)]
210214
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
215+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
211216
async fn apply_patch_cli_insert_only_hunk_modifies_file(
212217
model_output: ApplyPatchModelOutput,
213218
) -> Result<()> {
@@ -233,6 +238,7 @@ async fn apply_patch_cli_insert_only_hunk_modifies_file(
233238
#[test_case(ApplyPatchModelOutput::Function)]
234239
#[test_case(ApplyPatchModelOutput::Shell)]
235240
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
241+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
236242
async fn apply_patch_cli_move_overwrites_existing_destination(
237243
model_output: ApplyPatchModelOutput,
238244
) -> Result<()> {
@@ -263,6 +269,7 @@ async fn apply_patch_cli_move_overwrites_existing_destination(
263269
#[test_case(ApplyPatchModelOutput::Function)]
264270
#[test_case(ApplyPatchModelOutput::Shell)]
265271
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
272+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
266273
async fn apply_patch_cli_move_without_content_change_has_no_turn_diff(
267274
model_output: ApplyPatchModelOutput,
268275
) -> Result<()> {
@@ -320,6 +327,7 @@ async fn apply_patch_cli_move_without_content_change_has_no_turn_diff(
320327
#[test_case(ApplyPatchModelOutput::Function)]
321328
#[test_case(ApplyPatchModelOutput::Shell)]
322329
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
330+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
323331
async fn apply_patch_cli_add_overwrites_existing_file(
324332
model_output: ApplyPatchModelOutput,
325333
) -> Result<()> {
@@ -345,6 +353,7 @@ async fn apply_patch_cli_add_overwrites_existing_file(
345353
#[test_case(ApplyPatchModelOutput::Function)]
346354
#[test_case(ApplyPatchModelOutput::Shell)]
347355
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
356+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
348357
async fn apply_patch_cli_rejects_invalid_hunk_header(
349358
model_output: ApplyPatchModelOutput,
350359
) -> Result<()> {
@@ -376,6 +385,7 @@ async fn apply_patch_cli_rejects_invalid_hunk_header(
376385
#[test_case(ApplyPatchModelOutput::Function)]
377386
#[test_case(ApplyPatchModelOutput::Shell)]
378387
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
388+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
379389
async fn apply_patch_cli_reports_missing_context(
380390
model_output: ApplyPatchModelOutput,
381391
) -> Result<()> {
@@ -409,6 +419,7 @@ async fn apply_patch_cli_reports_missing_context(
409419
#[test_case(ApplyPatchModelOutput::Function)]
410420
#[test_case(ApplyPatchModelOutput::Shell)]
411421
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
422+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
412423
async fn apply_patch_cli_reports_missing_target_file(
413424
model_output: ApplyPatchModelOutput,
414425
) -> Result<()> {
@@ -444,6 +455,7 @@ async fn apply_patch_cli_reports_missing_target_file(
444455
#[test_case(ApplyPatchModelOutput::Function)]
445456
#[test_case(ApplyPatchModelOutput::Shell)]
446457
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
458+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
447459
async fn apply_patch_cli_delete_missing_file_reports_error(
448460
model_output: ApplyPatchModelOutput,
449461
) -> Result<()> {
@@ -480,6 +492,7 @@ async fn apply_patch_cli_delete_missing_file_reports_error(
480492
#[test_case(ApplyPatchModelOutput::Function)]
481493
#[test_case(ApplyPatchModelOutput::Shell)]
482494
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
495+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
483496
async fn apply_patch_cli_rejects_empty_patch(model_output: ApplyPatchModelOutput) -> Result<()> {
484497
skip_if_no_network!(Ok(()));
485498

@@ -504,6 +517,7 @@ async fn apply_patch_cli_rejects_empty_patch(model_output: ApplyPatchModelOutput
504517
#[test_case(ApplyPatchModelOutput::Function)]
505518
#[test_case(ApplyPatchModelOutput::Shell)]
506519
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
520+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
507521
async fn apply_patch_cli_delete_directory_reports_verification_error(
508522
model_output: ApplyPatchModelOutput,
509523
) -> Result<()> {
@@ -530,6 +544,7 @@ async fn apply_patch_cli_delete_directory_reports_verification_error(
530544
#[test_case(ApplyPatchModelOutput::Function)]
531545
#[test_case(ApplyPatchModelOutput::Shell)]
532546
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
547+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
533548
async fn apply_patch_cli_rejects_path_traversal_outside_workspace(
534549
model_output: ApplyPatchModelOutput,
535550
) -> Result<()> {
@@ -582,6 +597,7 @@ async fn apply_patch_cli_rejects_path_traversal_outside_workspace(
582597
#[test_case(ApplyPatchModelOutput::Function)]
583598
#[test_case(ApplyPatchModelOutput::Shell)]
584599
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
600+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
585601
async fn apply_patch_cli_rejects_move_path_traversal_outside_workspace(
586602
model_output: ApplyPatchModelOutput,
587603
) -> Result<()> {
@@ -635,6 +651,7 @@ async fn apply_patch_cli_rejects_move_path_traversal_outside_workspace(
635651
#[test_case(ApplyPatchModelOutput::Function)]
636652
#[test_case(ApplyPatchModelOutput::Shell)]
637653
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
654+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
638655
async fn apply_patch_cli_verification_failure_has_no_side_effects(
639656
model_output: ApplyPatchModelOutput,
640657
) -> Result<()> {
@@ -677,11 +694,10 @@ async fn apply_patch_shell_command_heredoc_with_cd_updates_relative_workdir() ->
677694

678695
let script = "cd sub && apply_patch <<'EOF'\n*** Begin Patch\n*** Update File: in_sub.txt\n@@\n-before\n+after\n*** End Patch\nEOF\n";
679696
let call_id = "shell-heredoc-cd";
680-
let args = json!({ "command": script, "timeout_ms": 5_000 });
681697
let bodies = vec![
682698
sse(vec![
683699
ev_response_created("resp-1"),
684-
ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?),
700+
ev_shell_command_call(call_id, script),
685701
ev_completed("resp-1"),
686702
]),
687703
sse(vec![
@@ -702,6 +718,86 @@ async fn apply_patch_shell_command_heredoc_with_cd_updates_relative_workdir() ->
702718
Ok(())
703719
}
704720

721+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
722+
async fn apply_patch_shell_command_heredoc_with_cd_emits_turn_diff() -> Result<()> {
723+
skip_if_no_network!(Ok(()));
724+
725+
let harness = apply_patch_harness_with(|builder| builder.with_model("gpt-5.1")).await?;
726+
let test = harness.test();
727+
let codex = test.codex.clone();
728+
let cwd = test.cwd.clone();
729+
730+
// Prepare a file inside a subdir; update it via cd && apply_patch heredoc form.
731+
let sub = test.workspace_path("sub");
732+
fs::create_dir_all(&sub)?;
733+
let target = sub.join("in_sub.txt");
734+
fs::write(&target, "before\n")?;
735+
736+
let script = "cd sub && apply_patch <<'EOF'\n*** Begin Patch\n*** Update File: in_sub.txt\n@@\n-before\n+after\n*** End Patch\nEOF\n";
737+
let call_id = "shell-heredoc-cd";
738+
let args = json!({ "command": script, "timeout_ms": 5_000 });
739+
let bodies = vec![
740+
sse(vec![
741+
ev_response_created("resp-1"),
742+
ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?),
743+
ev_completed("resp-1"),
744+
]),
745+
sse(vec![
746+
ev_assistant_message("msg-1", "ok"),
747+
ev_completed("resp-2"),
748+
]),
749+
];
750+
mount_sse_sequence(harness.server(), bodies).await;
751+
752+
let model = test.session_configured.model.clone();
753+
codex
754+
.submit(Op::UserTurn {
755+
items: vec![UserInput::Text {
756+
text: "apply via shell heredoc with cd".into(),
757+
}],
758+
final_output_json_schema: None,
759+
cwd: cwd.path().to_path_buf(),
760+
approval_policy: AskForApproval::Never,
761+
sandbox_policy: SandboxPolicy::DangerFullAccess,
762+
model,
763+
effort: None,
764+
summary: ReasoningSummary::Auto,
765+
})
766+
.await?;
767+
768+
let mut saw_turn_diff = None;
769+
let mut saw_patch_begin = false;
770+
let mut patch_end_success = None;
771+
wait_for_event(&codex, |event| match event {
772+
EventMsg::PatchApplyBegin(begin) => {
773+
saw_patch_begin = true;
774+
assert_eq!(begin.call_id, call_id);
775+
false
776+
}
777+
EventMsg::PatchApplyEnd(end) => {
778+
assert_eq!(end.call_id, call_id);
779+
patch_end_success = Some(end.success);
780+
false
781+
}
782+
EventMsg::TurnDiff(ev) => {
783+
saw_turn_diff = Some(ev.unified_diff.clone());
784+
false
785+
}
786+
EventMsg::TaskComplete(_) => true,
787+
_ => false,
788+
})
789+
.await;
790+
791+
assert!(saw_patch_begin, "expected PatchApplyBegin event");
792+
let patch_end_success =
793+
patch_end_success.expect("expected PatchApplyEnd event to capture success flag");
794+
assert!(patch_end_success);
795+
796+
let diff = saw_turn_diff.expect("expected TurnDiff event");
797+
assert!(diff.contains("diff --git"), "diff header missing: {diff:?}");
798+
Ok(())
799+
}
800+
705801
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
706802
async fn apply_patch_shell_command_failure_propagates_error_and_skips_diff() -> Result<()> {
707803
skip_if_no_network!(Ok(()));
@@ -776,24 +872,20 @@ async fn apply_patch_shell_command_failure_propagates_error_and_skips_diff() ->
776872
}
777873

778874
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
779-
async fn apply_patch_function_accepts_lenient_heredoc_wrapped_patch() -> Result<()> {
875+
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
876+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
877+
async fn apply_patch_function_accepts_lenient_heredoc_wrapped_patch(
878+
model_output: ApplyPatchModelOutput,
879+
) -> Result<()> {
780880
skip_if_no_network!(Ok(()));
781881

782882
let harness = apply_patch_harness().await?;
783883

784884
let file_name = "lenient.txt";
785885
let patch_inner =
786886
format!("*** Begin Patch\n*** Add File: {file_name}\n+lenient\n*** End Patch\n");
787-
let wrapped = format!("<<'EOF'\n{patch_inner}EOF\n");
788887
let call_id = "apply-lenient";
789-
mount_apply_patch(
790-
&harness,
791-
call_id,
792-
wrapped.as_str(),
793-
"ok",
794-
ApplyPatchModelOutput::Function,
795-
)
796-
.await;
888+
mount_apply_patch(&harness, call_id, patch_inner.as_str(), "ok", model_output).await;
797889

798890
harness.submit("apply lenient heredoc patch").await?;
799891

@@ -807,6 +899,7 @@ async fn apply_patch_function_accepts_lenient_heredoc_wrapped_patch() -> Result<
807899
#[test_case(ApplyPatchModelOutput::Function)]
808900
#[test_case(ApplyPatchModelOutput::Shell)]
809901
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
902+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
810903
async fn apply_patch_cli_end_of_file_anchor(model_output: ApplyPatchModelOutput) -> Result<()> {
811904
skip_if_no_network!(Ok(()));
812905

@@ -829,6 +922,7 @@ async fn apply_patch_cli_end_of_file_anchor(model_output: ApplyPatchModelOutput)
829922
#[test_case(ApplyPatchModelOutput::Function)]
830923
#[test_case(ApplyPatchModelOutput::Shell)]
831924
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
925+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
832926
async fn apply_patch_cli_missing_second_chunk_context_rejected(
833927
model_output: ApplyPatchModelOutput,
834928
) -> Result<()> {
@@ -863,6 +957,7 @@ async fn apply_patch_cli_missing_second_chunk_context_rejected(
863957
#[test_case(ApplyPatchModelOutput::Function)]
864958
#[test_case(ApplyPatchModelOutput::Shell)]
865959
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
960+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
866961
async fn apply_patch_emits_turn_diff_event_with_unified_diff(
867962
model_output: ApplyPatchModelOutput,
868963
) -> Result<()> {
@@ -918,6 +1013,7 @@ async fn apply_patch_emits_turn_diff_event_with_unified_diff(
9181013
#[test_case(ApplyPatchModelOutput::Function)]
9191014
#[test_case(ApplyPatchModelOutput::Shell)]
9201015
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
1016+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
9211017
async fn apply_patch_turn_diff_for_rename_with_content_change(
9221018
model_output: ApplyPatchModelOutput,
9231019
) -> Result<()> {
@@ -1132,6 +1228,7 @@ async fn apply_patch_aggregates_diff_preserves_success_after_failure() -> Result
11321228
#[test_case(ApplyPatchModelOutput::Function)]
11331229
#[test_case(ApplyPatchModelOutput::Shell)]
11341230
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
1231+
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
11351232
async fn apply_patch_change_context_disambiguates_target(
11361233
model_output: ApplyPatchModelOutput,
11371234
) -> Result<()> {

0 commit comments

Comments
 (0)