agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

[codex] Remove legacy shell output formatting paths (#22706)

Browse Source 
## Why

The client and tool pipeline still carried compatibility code for legacy
structured shell output. Current shell and apply_patch responses are
already plain text for model consumption, so keeping a
JSON-serialization path plus shell-item rewrite logic makes the request
formatter and tests preserve a format we do not need anymore.

## What Changed

- Removed the client-side shell output rewrite from
`core/src/client_common.rs`.
- Removed the structured exec-output formatter and the shell `freeform`
switch so tool emitters use one model-facing formatter.
- Collapsed apply_patch/shell serialization tests around the remaining
plain-text output expectations and removed duplicate one-variant
parameterized cases.
- Kept the `ApplyPatchModelOutput::ShellCommandViaHeredoc` compatibility
input shape, but no longer treats it as a separate output-format mode.

## Validation

- `cargo test -p codex-core client_common`
- `cargo test -p codex-core shell_serialization`
- `cargo test -p codex-core apply_patch_cli`
- `just fix -p codex-core`

## Documentation

No external Codex documentation update is needed.
This commit is contained in:
pakrym-oai
2026-05-18 09:57:54 -07:00
committed by GitHub
parent adca1b643f
commit 82061660ae
11 changed files with 186 additions and 707 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/tests/suite/shell_serialization.rs
+49 -236
View File
@@ -12,16 +12,12 @@ use core_test_support::responses::mount_sse_sequence;
use core_test_support::responses::sse;
use core_test_support::responses::start_mock_server;
use core_test_support::skip_if_no_network;
use core_test_support::test_codex::ApplyPatchModelOutput;
use core_test_support::test_codex::ShellModelOutput;
use core_test_support::test_codex::TestCodexBuilder;
use core_test_support::test_codex::test_codex;
use pretty_assertions::assert_eq;
use regex_lite::Regex;
use serde_json::Value;
use serde_json::json;
use std::fs;
use test_case::test_case;
use crate::suite::apply_patch_cli::apply_patch_harness;
use crate::suite::apply_patch_cli::mount_apply_patch;
@@ -38,135 +34,68 @@ const FIXTURE_JSON: &str = r#"{
}
"#;
fn shell_responses(
call_id: &str,
command: Vec<&str>,
output_type: ShellModelOutput,
) -> Result<Vec<String>> {
match output_type {
ShellModelOutput::ShellCommand => {
let command = shlex::try_join(command)?;
let parameters = json!({
"command": command,
"timeout_ms": 2_000,
});
Ok(vec![
sse(vec![
ev_response_created("resp-1"),
ev_function_call(
call_id,
"shell_command",
&serde_json::to_string(&parameters)?,
),
ev_completed("resp-1"),
]),
sse(vec![
ev_assistant_message("msg-1", "done"),
ev_completed("resp-2"),
]),
])
}
}
}
fn configure_shell_model(
builder: TestCodexBuilder,
output_type: ShellModelOutput,
) -> TestCodexBuilder {
match output_type {
ShellModelOutput::ShellCommand => builder.with_model("test-gpt-5-codex"),
}
fn shell_responses(call_id: &str, command: Vec<&str>) -> Result<Vec<String>> {
let command = shlex::try_join(command)?;
let parameters = json!({
"command": command,
"timeout_ms": 2_000,
});
Ok(vec![
sse(vec![
ev_response_created("resp-1"),
ev_function_call(
call_id,
"shell_command",
&serde_json::to_string(&parameters)?,
),
ev_completed("resp-1"),
]),
sse(vec![
ev_assistant_message("msg-1", "done"),
ev_completed("resp-2"),
]),
])
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ShellModelOutput::ShellCommand)]
async fn shell_output_is_structured_with_freeform_apply_patch(
output_type: ShellModelOutput,
) -> Result<()> {
async fn shell_output_preserves_fixture_json_as_freeform() -> Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
let mut builder = configure_shell_model(test_codex(), output_type);
let test = builder.build(&server).await?;
let call_id = "shell-structured";
let responses = shell_responses(call_id, vec!["/bin/echo", "freeform shell"], output_type)?;
let mock = mount_sse_sequence(&server, responses).await;
test.submit_turn_with_permission_profile(
"run the structured shell command",
PermissionProfile::Disabled,
)
.await?;
let req = mock
.last_request()
.expect("structured shell output request recorded");
let output_item = req.function_call_output(call_id);
let output = output_item
.get("output")
.and_then(Value::as_str)
.expect("structured output string");
assert!(
serde_json::from_str::<Value>(output).is_err(),
"expected structured shell output to be plain text",
);
let expected_pattern = r"(?s)^Exit code: 0
Wall time: [0-9]+(?:\.[0-9]+)? seconds
Output:
freeform shell
?$";
assert_regex_match(expected_pattern, output);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ShellModelOutput::ShellCommand)]
async fn shell_output_structures_fixture_with_serialization(
output_type: ShellModelOutput,
) -> Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
let mut builder = configure_shell_model(test_codex(), output_type);
let mut builder = test_codex().with_model("test-gpt-5-codex");
let test = builder.build(&server).await?;
let fixture_path = test.cwd.path().join("fixture.json");
fs::write(&fixture_path, FIXTURE_JSON)?;
let fixture_path_str = fixture_path.to_string_lossy().to_string();
let call_id = "shell-structured-fixture";
let call_id = "shell-freeform-fixture";
let responses = shell_responses(
call_id,
vec!["/usr/bin/sed", "-n", "p", fixture_path_str.as_str()],
output_type,
)?;
let mock = mount_sse_sequence(&server, responses).await;
test.submit_turn_with_permission_profile(
"read the fixture JSON with structured output",
"read the fixture JSON with shell output",
PermissionProfile::Disabled,
)
.await?;
let req = mock
.last_request()
.expect("structured output request recorded");
let req = mock.last_request().expect("shell output request recorded");
let output_item = req.function_call_output(call_id);
let output = output_item
.get("output")
.and_then(Value::as_str)
.expect("structured output string");
.expect("shell output string");
assert!(
serde_json::from_str::<Value>(output).is_err(),
"expected structured output to be plain text"
"expected shell output to be plain text"
);
let (header, body) = output
.split_once("Output:\n")
.expect("structured output contains an Output section");
.expect("shell output contains an Output section");
assert_regex_match(
r"(?s)^Exit code: 0\nWall time: [0-9]+(?:\.[0-9]+)? seconds$",
header.trim_end(),
@@ -180,34 +109,26 @@ async fn shell_output_structures_fixture_with_serialization(
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ShellModelOutput::ShellCommand)]
async fn shell_output_for_freeform_tool_records_duration(
output_type: ShellModelOutput,
) -> Result<()> {
async fn shell_output_records_duration() -> Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
let mut builder = configure_shell_model(test_codex(), output_type);
let mut builder = test_codex().with_model("test-gpt-5-codex");
let test = builder.build(&server).await?;
let call_id = "shell-structured";
let responses = shell_responses(call_id, vec!["/bin/sh", "-c", "sleep 0.2"], output_type)?;
let call_id = "shell-freeform";
let responses = shell_responses(call_id, vec!["/bin/sh", "-c", "sleep 0.2"])?;
let mock = mount_sse_sequence(&server, responses).await;
test.submit_turn_with_permission_profile(
"run the structured shell command",
PermissionProfile::Disabled,
)
.await?;
test.submit_turn_with_permission_profile("run the shell command", PermissionProfile::Disabled)
.await?;
let req = mock
.last_request()
.expect("structured output request recorded");
let req = mock.last_request().expect("shell output request recorded");
let output_item = req.function_call_output(call_id);
let output = output_item
.get("output")
.and_then(Value::as_str)
.expect("structured output string");
.expect("shell output string");
let expected_pattern = r#"(?s)^Exit code: 0
Wall time: [0-9]+(?:\.[0-9]+)? seconds
@@ -221,7 +142,7 @@ $"#;
.captures(output)
.and_then(|caps| caps.get(1))
.and_then(|value| value.as_str().parse::<f32>().ok())
.expect("expected structured shell output to contain wall time seconds");
.expect("expected shell output to contain wall time seconds");
assert!(
wall_time_seconds > 0.1,
"expected wall time to be greater than zero seconds, got {wall_time_seconds}"
@@ -231,53 +152,7 @@ $"#;
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ApplyPatchModelOutput::Freeform)]
async fn apply_patch_custom_tool_output_is_structured(
output_type: ApplyPatchModelOutput,
) -> Result<()> {
skip_if_no_network!(Ok(()));
let harness = apply_patch_harness().await?;
let call_id = "apply-patch-structured";
let file_name = "structured.txt";
let patch = format!(
r#"*** Begin Patch
*** Add File: {file_name}
+from custom tool
*** End Patch
"#
);
mount_apply_patch(&harness, call_id, &patch, "done", output_type).await;
harness
.test()
.submit_turn_with_permission_profile(
"apply the patch via custom tool",
PermissionProfile::Disabled,
)
.await?;
let output = harness.apply_patch_output(call_id, output_type).await;
let expected_pattern = format!(
r"(?s)^Exit code: 0
Wall time: [0-9]+(?:\.[0-9]+)? seconds
Output:
Success. Updated the following files:
A {file_name}
?$"
);
assert_regex_match(&expected_pattern, output.as_str());
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ApplyPatchModelOutput::Freeform)]
async fn apply_patch_custom_tool_call_creates_file(
output_type: ApplyPatchModelOutput,
) -> Result<()> {
async fn apply_patch_custom_tool_call_creates_file() -> Result<()> {
skip_if_no_network!(Ok(()));
let harness = apply_patch_harness().await?;
@@ -287,7 +162,7 @@ async fn apply_patch_custom_tool_call_creates_file(
let patch = format!(
"*** Begin Patch\n*** Add File: {file_name}\n+custom tool content\n*** End Patch\n"
);
mount_apply_patch(&harness, call_id, &patch, "apply_patch done", output_type).await;
mount_apply_patch(&harness, call_id, &patch, "apply_patch done").await;
harness
.test()
@@ -297,7 +172,7 @@ async fn apply_patch_custom_tool_call_creates_file(
)
.await?;
let output = harness.apply_patch_output(call_id, output_type).await;
let output = harness.apply_patch_output(call_id).await;
let expected_pattern = format!(
r"(?s)^Exit code: 0
@@ -319,10 +194,7 @@ A {file_name}
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ApplyPatchModelOutput::Freeform)]
async fn apply_patch_custom_tool_call_updates_existing_file(
output_type: ApplyPatchModelOutput,
) -> Result<()> {
async fn apply_patch_custom_tool_call_updates_existing_file() -> Result<()> {
skip_if_no_network!(Ok(()));
let harness = apply_patch_harness().await?;
@@ -333,14 +205,7 @@ async fn apply_patch_custom_tool_call_updates_existing_file(
let patch = format!(
"*** Begin Patch\n*** Update File: {file_name}\n@@\n-before\n+after\n*** End Patch\n"
);
mount_apply_patch(
&harness,
call_id,
&patch,
"apply_patch update done",
output_type,
)
.await;
mount_apply_patch(&harness, call_id, &patch, "apply_patch update done").await;
harness
.test()
@@ -350,7 +215,7 @@ async fn apply_patch_custom_tool_call_updates_existing_file(
)
.await?;
let output = harness.apply_patch_output(call_id, output_type).await;
let output = harness.apply_patch_output(call_id).await;
let expected_pattern = format!(
r"(?s)^Exit code: 0
@@ -369,10 +234,7 @@ M {file_name}
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ApplyPatchModelOutput::Freeform)]
async fn apply_patch_custom_tool_call_reports_failure_output(
output_type: ApplyPatchModelOutput,
) -> Result<()> {
async fn apply_patch_custom_tool_call_reports_failure_output() -> Result<()> {
skip_if_no_network!(Ok(()));
let harness = apply_patch_harness().await?;
@@ -382,14 +244,7 @@ async fn apply_patch_custom_tool_call_reports_failure_output(
let patch = format!(
"*** Begin Patch\n*** Update File: {missing_file}\n@@\n-before\n+after\n*** End Patch\n"
);
mount_apply_patch(
&harness,
call_id,
&patch,
"apply_patch failure done",
output_type,
)
.await;
mount_apply_patch(&harness, call_id, &patch, "apply_patch failure done").await;
harness
.test()
@@ -399,7 +254,7 @@ async fn apply_patch_custom_tool_call_reports_failure_output(
)
.await?;
let output = harness.apply_patch_output(call_id, output_type).await;
let output = harness.apply_patch_output(call_id).await;
let expected_output = format!(
"apply_patch verification failed: Failed to read file to update {}/{missing_file}: No such file or directory (os error 2)",
@@ -411,49 +266,7 @@ async fn apply_patch_custom_tool_call_reports_failure_output(
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ApplyPatchModelOutput::Freeform)]
async fn apply_patch_tool_output_is_structured(output_type: ApplyPatchModelOutput) -> Result<()> {
skip_if_no_network!(Ok(()));
let harness = apply_patch_harness().await?;
let call_id = "apply-patch-function";
let file_name = "freeform_apply_patch.txt";
let patch =
format!("*** Begin Patch\n*** Add File: {file_name}\n+via apply_patch\n*** End Patch\n");
mount_apply_patch(
&harness,
call_id,
&patch,
"apply_patch function done",
output_type,
)
.await;
harness
.test()
.submit_turn_with_permission_profile(
"apply the patch via freeform apply_patch",
PermissionProfile::Disabled,
)
.await?;
let output = harness.apply_patch_output(call_id, output_type).await;
let expected_pattern = format!(
r"(?s)^Exit code: 0
Wall time: [0-9]+(?:\.[0-9]+)? seconds
Output:
Success. Updated the following files:
A {file_name}
?$"
);
assert_regex_match(&expected_pattern, output.as_str());
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[test_case(ShellModelOutput::ShellCommand)]
async fn shell_output_is_structured_for_nonzero_exit(output_type: ShellModelOutput) -> Result<()> {
async fn shell_output_is_freeform_for_nonzero_exit() -> Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
@@ -461,7 +274,7 @@ async fn shell_output_is_structured_for_nonzero_exit(output_type: ShellModelOutp
let test = builder.build(&server).await?;
let call_id = "shell-nonzero-exit";
let responses = shell_responses(call_id, vec!["/bin/sh", "-c", "exit 42"], output_type)?;
let responses = shell_responses(call_id, vec!["/bin/sh", "-c", "exit 42"])?;
let mock = mount_sse_sequence(&server, responses).await;
test.submit_turn_with_permission_profile(