Skip to content

fix(apply_patch) tests for shell_command #7307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 1, 2025
+124 −13
Merged

fix(apply_patch) tests for shell_command #7307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01f58d1
fix(apply_patch) shell_command cd parsing
dylan-hurd-oai Nov 25, 2025
d7177ad
use helper
dylan-hurd-oai Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions codex-rs/core/tests/common/responses.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,9 @@ pub fn ev_apply_patch_call(
ApplyPatchModelOutput::ShellViaHeredoc => {
ev_apply_patch_shell_call_via_heredoc(call_id, patch)
}
ApplyPatchModelOutput::ShellCommandViaHeredoc => {
ev_apply_patch_shell_command_call_via_heredoc(call_id, patch)
}
}
}

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

pub fn ev_apply_patch_shell_command_call_via_heredoc(call_id: &str, patch: &str) -> Value {
let args = serde_json::json!({ "command": format!("apply_patch <<'EOF'\n{patch}\nEOF\n") });
let arguments = serde_json::to_string(&args).expect("serialize apply_patch arguments");

ev_function_call(call_id, "shell_command", &arguments)
}

pub fn sse_failed(id: &str, code: &str, message: &str) -> String {
sse(vec![serde_json::json!({
"type": "response.failed",
Expand Down
6 changes: 5 additions & 1 deletion codex-rs/core/tests/common/test_codex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ pub enum ApplyPatchModelOutput {
Function,
Shell,
ShellViaHeredoc,
ShellCommandViaHeredoc,
}

/// A collection of different ways the model can output an apply_patch call
Expand Down Expand Up @@ -312,7 +313,10 @@ impl TestCodexHarness {
ApplyPatchModelOutput::Freeform => self.custom_tool_call_output(call_id).await,
ApplyPatchModelOutput::Function
| ApplyPatchModelOutput::Shell
| ApplyPatchModelOutput::ShellViaHeredoc => self.function_call_stdout(call_id).await,
| ApplyPatchModelOutput::ShellViaHeredoc
| ApplyPatchModelOutput::ShellCommandViaHeredoc => {
self.function_call_stdout(call_id).await
}
}
}
}
Expand Down
121 changes: 109 additions & 12 deletions codex-rs/core/tests/suite/apply_patch_cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use anyhow::Result;
use core_test_support::responses::ev_apply_patch_call;
use core_test_support::responses::ev_shell_command_call;
use core_test_support::test_codex::ApplyPatchModelOutput;
use pretty_assertions::assert_eq;
use std::fs;
Expand Down Expand Up @@ -127,6 +128,7 @@ D delete.txt
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_multiple_chunks(model_output: ApplyPatchModelOutput) -> Result<()> {
skip_if_no_network!(Ok(()));

Expand All @@ -153,6 +155,7 @@ async fn apply_patch_cli_multiple_chunks(model_output: ApplyPatchModelOutput) ->
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_moves_file_to_new_directory(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -181,6 +184,7 @@ async fn apply_patch_cli_moves_file_to_new_directory(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_updates_file_appends_trailing_newline(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -208,6 +212,7 @@ async fn apply_patch_cli_updates_file_appends_trailing_newline(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_insert_only_hunk_modifies_file(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand All @@ -233,6 +238,7 @@ async fn apply_patch_cli_insert_only_hunk_modifies_file(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_move_overwrites_existing_destination(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -263,6 +269,7 @@ async fn apply_patch_cli_move_overwrites_existing_destination(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_move_without_content_change_has_no_turn_diff(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -320,6 +327,7 @@ async fn apply_patch_cli_move_without_content_change_has_no_turn_diff(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_add_overwrites_existing_file(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand All @@ -345,6 +353,7 @@ async fn apply_patch_cli_add_overwrites_existing_file(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_rejects_invalid_hunk_header(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -376,6 +385,7 @@ async fn apply_patch_cli_rejects_invalid_hunk_header(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_reports_missing_context(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -409,6 +419,7 @@ async fn apply_patch_cli_reports_missing_context(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_reports_missing_target_file(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -444,6 +455,7 @@ async fn apply_patch_cli_reports_missing_target_file(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_delete_missing_file_reports_error(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -480,6 +492,7 @@ async fn apply_patch_cli_delete_missing_file_reports_error(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_rejects_empty_patch(model_output: ApplyPatchModelOutput) -> Result<()> {
skip_if_no_network!(Ok(()));

Expand All @@ -504,6 +517,7 @@ async fn apply_patch_cli_rejects_empty_patch(model_output: ApplyPatchModelOutput
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_delete_directory_reports_verification_error(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand All @@ -530,6 +544,7 @@ async fn apply_patch_cli_delete_directory_reports_verification_error(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_rejects_path_traversal_outside_workspace(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -582,6 +597,7 @@ async fn apply_patch_cli_rejects_path_traversal_outside_workspace(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_rejects_move_path_traversal_outside_workspace(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -635,6 +651,7 @@ async fn apply_patch_cli_rejects_move_path_traversal_outside_workspace(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_verification_failure_has_no_side_effects(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -677,11 +694,10 @@ async fn apply_patch_shell_command_heredoc_with_cd_updates_relative_workdir() ->

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";
let call_id = "shell-heredoc-cd";
let args = json!({ "command": script, "timeout_ms": 5_000 });
let bodies = vec![
sse(vec![
ev_response_created("resp-1"),
ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?),
ev_shell_command_call(call_id, script),
ev_completed("resp-1"),
]),
sse(vec![
Expand All @@ -702,6 +718,86 @@ async fn apply_patch_shell_command_heredoc_with_cd_updates_relative_workdir() ->
Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn apply_patch_shell_command_heredoc_with_cd_emits_turn_diff() -> Result<()> {
skip_if_no_network!(Ok(()));

let harness = apply_patch_harness_with(|builder| builder.with_model("gpt-5.1")).await?;
let test = harness.test();
let codex = test.codex.clone();
let cwd = test.cwd.clone();

// Prepare a file inside a subdir; update it via cd && apply_patch heredoc form.
let sub = test.workspace_path("sub");
fs::create_dir_all(&sub)?;
let target = sub.join("in_sub.txt");
fs::write(&target, "before\n")?;

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";
let call_id = "shell-heredoc-cd";
let args = json!({ "command": script, "timeout_ms": 5_000 });
let bodies = vec![
sse(vec![
ev_response_created("resp-1"),
ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?),
Copy link
Collaborator

@pakrym-oai pakrym-oai Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't there a helper for this?

ev_completed("resp-1"),
]),
sse(vec![
ev_assistant_message("msg-1", "ok"),
ev_completed("resp-2"),
]),
];
mount_sse_sequence(harness.server(), bodies).await;

let model = test.session_configured.model.clone();
codex
.submit(Op::UserTurn {
items: vec![UserInput::Text {
text: "apply via shell heredoc with cd".into(),
}],
final_output_json_schema: None,
cwd: cwd.path().to_path_buf(),
approval_policy: AskForApproval::Never,
sandbox_policy: SandboxPolicy::DangerFullAccess,
model,
effort: None,
summary: ReasoningSummary::Auto,
})
.await?;

let mut saw_turn_diff = None;
let mut saw_patch_begin = false;
let mut patch_end_success = None;
wait_for_event(&codex, |event| match event {
EventMsg::PatchApplyBegin(begin) => {
saw_patch_begin = true;
assert_eq!(begin.call_id, call_id);
false
}
EventMsg::PatchApplyEnd(end) => {
assert_eq!(end.call_id, call_id);
patch_end_success = Some(end.success);
false
}
EventMsg::TurnDiff(ev) => {
saw_turn_diff = Some(ev.unified_diff.clone());
false
}
EventMsg::TaskComplete(_) => true,
_ => false,
})
.await;

assert!(saw_patch_begin, "expected PatchApplyBegin event");
let patch_end_success =
patch_end_success.expect("expected PatchApplyEnd event to capture success flag");
assert!(patch_end_success);

let diff = saw_turn_diff.expect("expected TurnDiff event");
assert!(diff.contains("diff --git"), "diff header missing: {diff:?}");
Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn apply_patch_shell_command_failure_propagates_error_and_skips_diff() -> Result<()> {
skip_if_no_network!(Ok(()));
Expand Down Expand Up @@ -776,24 +872,20 @@ async fn apply_patch_shell_command_failure_propagates_error_and_skips_diff() ->
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn apply_patch_function_accepts_lenient_heredoc_wrapped_patch() -> Result<()> {
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_function_accepts_lenient_heredoc_wrapped_patch(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
skip_if_no_network!(Ok(()));

let harness = apply_patch_harness().await?;

let file_name = "lenient.txt";
let patch_inner =
format!("*** Begin Patch\n*** Add File: {file_name}\n+lenient\n*** End Patch\n");
let wrapped = format!("<<'EOF'\n{patch_inner}EOF\n");
let call_id = "apply-lenient";
mount_apply_patch(
&harness,
call_id,
wrapped.as_str(),
"ok",
ApplyPatchModelOutput::Function,
)
.await;
mount_apply_patch(&harness, call_id, patch_inner.as_str(), "ok", model_output).await;

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

Expand All @@ -807,6 +899,7 @@ async fn apply_patch_function_accepts_lenient_heredoc_wrapped_patch() -> Result<
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_end_of_file_anchor(model_output: ApplyPatchModelOutput) -> Result<()> {
skip_if_no_network!(Ok(()));

Expand All @@ -829,6 +922,7 @@ async fn apply_patch_cli_end_of_file_anchor(model_output: ApplyPatchModelOutput)
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_cli_missing_second_chunk_context_rejected(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -863,6 +957,7 @@ async fn apply_patch_cli_missing_second_chunk_context_rejected(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_emits_turn_diff_event_with_unified_diff(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -918,6 +1013,7 @@ async fn apply_patch_emits_turn_diff_event_with_unified_diff(
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_turn_diff_for_rename_with_content_change(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down Expand Up @@ -1132,6 +1228,7 @@ async fn apply_patch_aggregates_diff_preserves_success_after_failure() -> Result
#[test_case(ApplyPatchModelOutput::Function)]
#[test_case(ApplyPatchModelOutput::Shell)]
#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)]
#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)]
async fn apply_patch_change_context_disambiguates_target(
model_output: ApplyPatchModelOutput,
) -> Result<()> {
Expand Down
Loading