mirror of
https://github.com/openai/codex.git
synced 2026-09-13 11:47:17 +00:00
feat: custom-apply-patch with text shell output
This commit is contained in:
@@ -2400,6 +2400,7 @@ mod tests {
|
||||
use crate::state::TaskKind;
|
||||
use crate::tasks::SessionTask;
|
||||
use crate::tasks::SessionTaskContext;
|
||||
use crate::tools::ExecResponseFormat;
|
||||
use crate::tools::MODEL_FORMAT_HEAD_LINES;
|
||||
use crate::tools::MODEL_FORMAT_MAX_BYTES;
|
||||
use crate::tools::MODEL_FORMAT_MAX_LINES;
|
||||
@@ -3089,6 +3090,7 @@ mod tests {
|
||||
&mut turn_diff_tracker,
|
||||
sub_id,
|
||||
call_id,
|
||||
ExecResponseFormat::LegacyJson,
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -3115,6 +3117,7 @@ mod tests {
|
||||
&mut turn_diff_tracker,
|
||||
"test-sub".to_string(),
|
||||
"test-call-2".to_string(),
|
||||
ExecResponseFormat::LegacyJson,
|
||||
)
|
||||
.await;
|
||||
|
||||
|
||||
@@ -8,6 +8,7 @@ use crate::client_common::tools::ToolSpec;
|
||||
use crate::exec::ExecParams;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::openai_tools::JsonSchema;
|
||||
use crate::tools::ExecResponseFormat;
|
||||
use crate::tools::context::ToolInvocation;
|
||||
use crate::tools::context::ToolOutput;
|
||||
use crate::tools::context::ToolPayload;
|
||||
@@ -50,16 +51,16 @@ impl ToolHandler for ApplyPatchHandler {
|
||||
payload,
|
||||
} = invocation;
|
||||
|
||||
let patch_input = match payload {
|
||||
let (patch_input, response_format) = match payload {
|
||||
ToolPayload::Function { arguments } => {
|
||||
let args: ApplyPatchToolArgs = serde_json::from_str(&arguments).map_err(|e| {
|
||||
FunctionCallError::RespondToModel(format!(
|
||||
"failed to parse function arguments: {e:?}"
|
||||
))
|
||||
})?;
|
||||
args.input
|
||||
(args.input, ExecResponseFormat::LegacyJson)
|
||||
}
|
||||
ToolPayload::Custom { input } => input,
|
||||
ToolPayload::Custom { input } => (input, ExecResponseFormat::StructuredText),
|
||||
_ => {
|
||||
return Err(FunctionCallError::RespondToModel(
|
||||
"apply_patch handler received unsupported payload".to_string(),
|
||||
@@ -84,6 +85,7 @@ impl ToolHandler for ApplyPatchHandler {
|
||||
tracker,
|
||||
sub_id.to_string(),
|
||||
call_id.clone(),
|
||||
response_format,
|
||||
)
|
||||
.await?;
|
||||
|
||||
|
||||
@@ -5,10 +5,12 @@ use crate::codex::TurnContext;
|
||||
use crate::exec::ExecParams;
|
||||
use crate::exec_env::create_env;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::tools::ExecResponseFormat;
|
||||
use crate::tools::context::ToolInvocation;
|
||||
use crate::tools::context::ToolOutput;
|
||||
use crate::tools::context::ToolPayload;
|
||||
use crate::tools::handle_container_exec_with_params;
|
||||
use crate::tools::handlers::apply_patch::ApplyPatchToolType;
|
||||
use crate::tools::registry::ToolHandler;
|
||||
use crate::tools::registry::ToolKind;
|
||||
|
||||
@@ -54,6 +56,14 @@ impl ToolHandler for ShellHandler {
|
||||
payload,
|
||||
} = invocation;
|
||||
|
||||
// When using the freeform apply_patch tool type, shell tool output should also be raw
|
||||
// output, not json-encoded.
|
||||
let response_format = match turn.tools_config.apply_patch_tool_type {
|
||||
Some(ApplyPatchToolType::Freeform) => ExecResponseFormat::StructuredText,
|
||||
Some(ApplyPatchToolType::Function) => ExecResponseFormat::LegacyJson,
|
||||
None => ExecResponseFormat::LegacyJson,
|
||||
};
|
||||
|
||||
match payload {
|
||||
ToolPayload::Function { arguments } => {
|
||||
let params: ShellToolCallParams =
|
||||
@@ -71,6 +81,7 @@ impl ToolHandler for ShellHandler {
|
||||
tracker,
|
||||
sub_id.to_string(),
|
||||
call_id.clone(),
|
||||
response_format,
|
||||
)
|
||||
.await?;
|
||||
Ok(ToolOutput::Function {
|
||||
@@ -88,6 +99,7 @@ impl ToolHandler for ShellHandler {
|
||||
tracker,
|
||||
sub_id.to_string(),
|
||||
call_id.clone(),
|
||||
response_format,
|
||||
)
|
||||
.await?;
|
||||
Ok(ToolOutput::Function {
|
||||
|
||||
@@ -44,6 +44,12 @@ pub(crate) const TELEMETRY_PREVIEW_MAX_LINES: usize = 64; // lines
|
||||
pub(crate) const TELEMETRY_PREVIEW_TRUNCATION_NOTICE: &str =
|
||||
"[... telemetry preview truncated ...]";
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
pub(crate) enum ExecResponseFormat {
|
||||
LegacyJson,
|
||||
StructuredText,
|
||||
}
|
||||
|
||||
// TODO(jif) break this down
|
||||
pub(crate) async fn handle_container_exec_with_params(
|
||||
tool_name: &str,
|
||||
@@ -53,6 +59,7 @@ pub(crate) async fn handle_container_exec_with_params(
|
||||
turn_diff_tracker: &mut TurnDiffTracker,
|
||||
sub_id: String,
|
||||
call_id: String,
|
||||
response_format: ExecResponseFormat,
|
||||
) -> Result<String, FunctionCallError> {
|
||||
let otel_event_manager = turn_context.client.get_otel_event_manager();
|
||||
|
||||
@@ -148,7 +155,7 @@ pub(crate) async fn handle_container_exec_with_params(
|
||||
match output_result {
|
||||
Ok(output) => {
|
||||
let ExecToolCallOutput { exit_code, .. } = &output;
|
||||
let content = format_exec_output_apply_patch(&output);
|
||||
let content = format_exec_output_with_style(&output, response_format);
|
||||
if *exit_code == 0 {
|
||||
Ok(content)
|
||||
} else {
|
||||
@@ -156,12 +163,14 @@ pub(crate) async fn handle_container_exec_with_params(
|
||||
}
|
||||
}
|
||||
Err(ExecError::Function(err)) => Err(err),
|
||||
Err(ExecError::Codex(CodexErr::Sandbox(SandboxErr::Timeout { output }))) => Err(
|
||||
FunctionCallError::RespondToModel(format_exec_output_apply_patch(&output)),
|
||||
),
|
||||
Err(ExecError::Codex(err)) => Err(FunctionCallError::RespondToModel(format!(
|
||||
"execution error: {err:?}"
|
||||
))),
|
||||
Err(ExecError::Codex(CodexErr::Sandbox(SandboxErr::Timeout { output }))) => {
|
||||
Err(FunctionCallError::RespondToModel(
|
||||
format_exec_output_with_style(&output, response_format),
|
||||
))
|
||||
}
|
||||
Err(ExecError::Codex(err)) => Err(FunctionCallError::RespondToModel(
|
||||
format_unexpected_exec_error(err, response_format),
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -201,6 +210,155 @@ pub fn format_exec_output_apply_patch(exec_output: &ExecToolCallOutput) -> Strin
|
||||
serde_json::to_string(&payload).expect("serialize ExecOutput")
|
||||
}
|
||||
|
||||
fn format_exec_output_with_style(
|
||||
exec_output: &ExecToolCallOutput,
|
||||
response_format: ExecResponseFormat,
|
||||
) -> String {
|
||||
match response_format {
|
||||
ExecResponseFormat::LegacyJson => format_exec_output_apply_patch(exec_output),
|
||||
ExecResponseFormat::StructuredText => format_exec_output_structured(exec_output),
|
||||
}
|
||||
}
|
||||
|
||||
fn format_unexpected_exec_error(err: CodexErr, response_format: ExecResponseFormat) -> String {
|
||||
match response_format {
|
||||
ExecResponseFormat::LegacyJson => format!("execution error: {err:?}"),
|
||||
ExecResponseFormat::StructuredText => format_structured_error(&format!("{err:?}")),
|
||||
}
|
||||
}
|
||||
|
||||
fn format_structured_error(message: &str) -> String {
|
||||
let lines = vec![
|
||||
"Exit code: N/A".to_string(),
|
||||
"Wall time: N/A seconds".to_string(),
|
||||
format!("Error: {message}"),
|
||||
"Output:".to_string(),
|
||||
String::new(),
|
||||
];
|
||||
lines.join("\n")
|
||||
}
|
||||
|
||||
fn format_wall_time(duration: std::time::Duration) -> String {
|
||||
format_significant_digits(duration.as_secs_f64(), 4)
|
||||
}
|
||||
|
||||
fn format_significant_digits(value: f64, digits: usize) -> String {
|
||||
if !value.is_finite() {
|
||||
return value.to_string();
|
||||
}
|
||||
if value == 0.0 {
|
||||
return "0".to_string();
|
||||
}
|
||||
|
||||
let abs = value.abs();
|
||||
let initial_exponent = abs.log10().floor() as i32;
|
||||
let rounded_value = if value == 0.0 {
|
||||
0.0
|
||||
} else {
|
||||
let scale = 10_f64.powf((digits as f64 - 1.0) - initial_exponent as f64);
|
||||
(value * scale).round() / scale
|
||||
};
|
||||
|
||||
let abs_rounded = rounded_value.abs();
|
||||
let exponent = if abs_rounded == 0.0 {
|
||||
0
|
||||
} else {
|
||||
abs_rounded.log10().floor() as i32
|
||||
};
|
||||
let use_exp = exponent < -4 || exponent >= digits as i32;
|
||||
if use_exp {
|
||||
return format!("{rounded_value:.prec$e}", prec = digits.saturating_sub(1));
|
||||
}
|
||||
|
||||
let decimal_places = (digits as i32 - exponent - 1).max(0) as usize;
|
||||
let mut s = format!("{rounded_value:.decimal_places$}");
|
||||
if s.contains('.') {
|
||||
while s.ends_with('0') {
|
||||
s.pop();
|
||||
}
|
||||
if s.ends_with('.') {
|
||||
s.pop();
|
||||
}
|
||||
}
|
||||
s
|
||||
}
|
||||
|
||||
pub fn format_exec_output_structured(exec_output: &ExecToolCallOutput) -> String {
|
||||
let ExecToolCallOutput {
|
||||
exit_code,
|
||||
duration,
|
||||
aggregated_output,
|
||||
..
|
||||
} = exec_output;
|
||||
|
||||
let mut sections = Vec::new();
|
||||
sections.push(format!("Exit code: {exit_code}"));
|
||||
sections.push(format!(
|
||||
"Wall time: {} seconds",
|
||||
format_wall_time(*duration)
|
||||
));
|
||||
|
||||
if let Some(total_lines) = aggregated_output.truncated_after_lines {
|
||||
sections.push(format!("Total output lines: {total_lines}"));
|
||||
}
|
||||
|
||||
sections.push("Output:".to_string());
|
||||
sections.push(format_exec_output_str(exec_output));
|
||||
|
||||
sections.join("\n")
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::exec::StreamOutput;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::time::Duration;
|
||||
|
||||
fn sample_output() -> ExecToolCallOutput {
|
||||
ExecToolCallOutput {
|
||||
exit_code: 0,
|
||||
stdout: StreamOutput::new("stdout".to_string()),
|
||||
stderr: StreamOutput::new("stderr".to_string()),
|
||||
aggregated_output: StreamOutput::new("stdout\nstderr".to_string()),
|
||||
duration: Duration::from_secs_f64(1.2345),
|
||||
timed_out: false,
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn structured_format_basic() {
|
||||
let formatted = format_exec_output_structured(&sample_output());
|
||||
let expected = "Exit code: 0\nWall time: 1.235 seconds\nOutput:\nstdout\nstderr";
|
||||
assert_eq!(formatted, expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn structured_format_includes_truncation_metadata() {
|
||||
let mut output = sample_output();
|
||||
output.aggregated_output.truncated_after_lines = Some(200);
|
||||
let formatted = format_exec_output_structured(&output);
|
||||
assert!(formatted.contains("Total output lines: 200"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn significant_digit_formatting_matches_expectations() {
|
||||
assert_eq!(format_significant_digits(0.0, 4), "0");
|
||||
assert_eq!(format_significant_digits(1.23456, 4), "1.235");
|
||||
assert_eq!(format_significant_digits(12345.0, 4), "1.235e4");
|
||||
assert_eq!(format_significant_digits(0.000123456, 4), "0.0001235");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn structured_error_includes_metadata() {
|
||||
let error = format_structured_error("unexpected failure");
|
||||
assert_eq!(
|
||||
error,
|
||||
"Exit code: N/A\nWall time: N/A seconds\nError: unexpected failure\nOutput:\n"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
pub fn format_exec_output_str(exec_output: &ExecToolCallOutput) -> String {
|
||||
let ExecToolCallOutput {
|
||||
aggregated_output, ..
|
||||
|
||||
@@ -71,6 +71,10 @@ pub struct Cli {
|
||||
#[arg(long = "include-plan-tool", default_value_t = false)]
|
||||
pub include_plan_tool: bool,
|
||||
|
||||
/// Force-enable the apply_patch tool even for models that do not opt into it by default.
|
||||
#[arg(long = "custom-apply-patch", default_value_t = false)]
|
||||
pub custom_apply_patch: bool,
|
||||
|
||||
/// Specifies file where the last message from the agent should be written.
|
||||
#[arg(long = "output-last-message", short = 'o', value_name = "FILE")]
|
||||
pub last_message_file: Option<PathBuf>,
|
||||
|
||||
@@ -69,6 +69,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
||||
prompt,
|
||||
output_schema: output_schema_path,
|
||||
include_plan_tool,
|
||||
custom_apply_patch,
|
||||
config_overrides,
|
||||
} = cli;
|
||||
|
||||
@@ -177,7 +178,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
||||
codex_linux_sandbox_exe,
|
||||
base_instructions: None,
|
||||
include_plan_tool: Some(include_plan_tool),
|
||||
include_apply_patch_tool: Some(true),
|
||||
include_apply_patch_tool: custom_apply_patch.then_some(true),
|
||||
include_view_image_tool: None,
|
||||
show_raw_agent_reasoning: oss.then_some(true),
|
||||
tools_web_search_request: None,
|
||||
|
||||
@@ -72,6 +72,10 @@ pub struct Cli {
|
||||
#[arg(long = "search", default_value_t = false)]
|
||||
pub web_search: bool,
|
||||
|
||||
/// Force-enable the apply_patch tool even for models that do not opt into it by default.
|
||||
#[arg(long = "custom-apply-patch", default_value_t = false)]
|
||||
pub custom_apply_patch: bool,
|
||||
|
||||
#[clap(skip)]
|
||||
pub config_overrides: CliConfigOverrides,
|
||||
}
|
||||
|
||||
@@ -142,7 +142,7 @@ pub async fn run_main(
|
||||
codex_linux_sandbox_exe,
|
||||
base_instructions: None,
|
||||
include_plan_tool: Some(true),
|
||||
include_apply_patch_tool: None,
|
||||
include_apply_patch_tool: cli.custom_apply_patch.then_some(true),
|
||||
include_view_image_tool: None,
|
||||
show_raw_agent_reasoning: cli.oss.then_some(true),
|
||||
tools_web_search_request: cli.web_search.then_some(true),
|
||||
|
||||
Reference in New Issue
Block a user