chore: refactor handle_function_call() into smaller functions (#965)
Overall, `codex.rs` is still far too large, but at least there's less indenting now that things have been moved into smaller functions. This will also make it easier to introduce the `local_shell` tool in https://github.com/openai/codex/pull/961. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/965). * #961 * __->__ #965
This commit is contained in:
@@ -40,6 +40,7 @@ use crate::config::Config;
|
|||||||
use crate::conversation_history::ConversationHistory;
|
use crate::conversation_history::ConversationHistory;
|
||||||
use crate::error::CodexErr;
|
use crate::error::CodexErr;
|
||||||
use crate::error::Result as CodexResult;
|
use crate::error::Result as CodexResult;
|
||||||
|
use crate::error::SandboxErr;
|
||||||
use crate::exec::ExecParams;
|
use crate::exec::ExecParams;
|
||||||
use crate::exec::ExecToolCallOutput;
|
use crate::exec::ExecToolCallOutput;
|
||||||
use crate::exec::SandboxType;
|
use crate::exec::SandboxType;
|
||||||
@@ -1042,26 +1043,75 @@ async fn handle_function_call(
|
|||||||
) -> ResponseInputItem {
|
) -> ResponseInputItem {
|
||||||
match name.as_str() {
|
match name.as_str() {
|
||||||
"container.exec" | "shell" => {
|
"container.exec" | "shell" => {
|
||||||
// parse command
|
let params = match parse_container_exec_arguments(arguments, sess, &call_id) {
|
||||||
let params: ExecParams = match serde_json::from_str::<ShellToolCallParams>(&arguments) {
|
Ok(params) => params,
|
||||||
Ok(shell_tool_call_params) => ExecParams {
|
Err(output) => {
|
||||||
command: shell_tool_call_params.command,
|
return output;
|
||||||
cwd: sess.resolve_path(shell_tool_call_params.workdir.clone()),
|
}
|
||||||
timeout_ms: shell_tool_call_params.timeout_ms,
|
};
|
||||||
|
handle_container_exec_with_params(params, sess, sub_id, call_id).await
|
||||||
|
}
|
||||||
|
_ => {
|
||||||
|
match try_parse_fully_qualified_tool_name(&name) {
|
||||||
|
Some((server, tool_name)) => {
|
||||||
|
// TODO(mbolin): Determine appropriate timeout for tool call.
|
||||||
|
let timeout = None;
|
||||||
|
handle_mcp_tool_call(
|
||||||
|
sess, &sub_id, call_id, server, tool_name, arguments, timeout,
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
}
|
||||||
|
None => {
|
||||||
|
// Unknown function: reply with structured failure so the model can adapt.
|
||||||
|
ResponseInputItem::FunctionCallOutput {
|
||||||
|
call_id,
|
||||||
|
output: crate::models::FunctionCallOutputPayload {
|
||||||
|
content: format!("unsupported call: {}", name),
|
||||||
|
success: None,
|
||||||
},
|
},
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn to_exec_params(params: ShellToolCallParams, sess: &Session) -> ExecParams {
|
||||||
|
ExecParams {
|
||||||
|
command: params.command,
|
||||||
|
cwd: sess.resolve_path(params.workdir.clone()),
|
||||||
|
timeout_ms: params.timeout_ms,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn parse_container_exec_arguments(
|
||||||
|
arguments: String,
|
||||||
|
sess: &Session,
|
||||||
|
call_id: &str,
|
||||||
|
) -> Result<ExecParams, ResponseInputItem> {
|
||||||
|
// parse command
|
||||||
|
match serde_json::from_str::<ShellToolCallParams>(&arguments) {
|
||||||
|
Ok(shell_tool_call_params) => Ok(to_exec_params(shell_tool_call_params, sess)),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
// allow model to re-sample
|
// allow model to re-sample
|
||||||
let output = ResponseInputItem::FunctionCallOutput {
|
let output = ResponseInputItem::FunctionCallOutput {
|
||||||
call_id,
|
call_id: call_id.to_string(),
|
||||||
output: crate::models::FunctionCallOutputPayload {
|
output: crate::models::FunctionCallOutputPayload {
|
||||||
content: format!("failed to parse function arguments: {e}"),
|
content: format!("failed to parse function arguments: {e}"),
|
||||||
success: None,
|
success: None,
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
return output;
|
Err(output)
|
||||||
}
|
}
|
||||||
};
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
async fn handle_container_exec_with_params(
|
||||||
|
params: ExecParams,
|
||||||
|
sess: &Session,
|
||||||
|
sub_id: String,
|
||||||
|
call_id: String,
|
||||||
|
) -> ResponseInputItem {
|
||||||
// check if this was a patch, and apply it if so
|
// check if this was a patch, and apply it if so
|
||||||
match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) {
|
match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) {
|
||||||
MaybeApplyPatchVerified::Body(changes) => {
|
MaybeApplyPatchVerified::Body(changes) => {
|
||||||
@@ -1176,14 +1226,37 @@ async fn handle_function_call(
|
|||||||
},
|
},
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Err(CodexErr::Sandbox(e)) => {
|
Err(CodexErr::Sandbox(error)) => {
|
||||||
|
handle_sanbox_error(error, sandbox_type, params, sess, sub_id, call_id).await
|
||||||
|
}
|
||||||
|
Err(e) => {
|
||||||
|
// Handle non-sandbox errors
|
||||||
|
ResponseInputItem::FunctionCallOutput {
|
||||||
|
call_id,
|
||||||
|
output: FunctionCallOutputPayload {
|
||||||
|
content: format!("execution error: {e}"),
|
||||||
|
success: None,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
async fn handle_sanbox_error(
|
||||||
|
error: SandboxErr,
|
||||||
|
sandbox_type: SandboxType,
|
||||||
|
params: ExecParams,
|
||||||
|
sess: &Session,
|
||||||
|
sub_id: String,
|
||||||
|
call_id: String,
|
||||||
|
) -> ResponseInputItem {
|
||||||
// Early out if the user never wants to be asked for approval; just return to the model immediately
|
// Early out if the user never wants to be asked for approval; just return to the model immediately
|
||||||
if sess.approval_policy == AskForApproval::Never {
|
if sess.approval_policy == AskForApproval::Never {
|
||||||
return ResponseInputItem::FunctionCallOutput {
|
return ResponseInputItem::FunctionCallOutput {
|
||||||
call_id,
|
call_id,
|
||||||
output: FunctionCallOutputPayload {
|
output: FunctionCallOutputPayload {
|
||||||
content: format!(
|
content: format!(
|
||||||
"failed in sandbox {:?} with execution error: {e}",
|
"failed in sandbox {:?} with execution error: {error}",
|
||||||
sandbox_type
|
sandbox_type
|
||||||
),
|
),
|
||||||
success: Some(false),
|
success: Some(false),
|
||||||
@@ -1192,7 +1265,7 @@ async fn handle_function_call(
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Ask the user to retry without sandbox
|
// Ask the user to retry without sandbox
|
||||||
sess.notify_background_event(&sub_id, format!("Execution failed: {e}"))
|
sess.notify_background_event(&sub_id, format!("Execution failed: {error}"))
|
||||||
.await;
|
.await;
|
||||||
|
|
||||||
let rx_approve = sess
|
let rx_approve = sess
|
||||||
@@ -1212,10 +1285,7 @@ async fn handle_function_call(
|
|||||||
// TODO(ragona): Isn't this a bug? It always saves the command in an | fork?
|
// TODO(ragona): Isn't this a bug? It always saves the command in an | fork?
|
||||||
sess.add_approved_command(params.command.clone());
|
sess.add_approved_command(params.command.clone());
|
||||||
// Inform UI we are retrying without sandbox.
|
// Inform UI we are retrying without sandbox.
|
||||||
sess.notify_background_event(
|
sess.notify_background_event(&sub_id, "retrying command without sandbox")
|
||||||
&sub_id,
|
|
||||||
"retrying command without sandbox",
|
|
||||||
)
|
|
||||||
.await;
|
.await;
|
||||||
|
|
||||||
// Emit a fresh Begin event so progress bars reset.
|
// Emit a fresh Begin event so progress bars reset.
|
||||||
@@ -1289,42 +1359,6 @@ async fn handle_function_call(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
|
||||||
Err(e) => {
|
|
||||||
// Handle non-sandbox errors
|
|
||||||
ResponseInputItem::FunctionCallOutput {
|
|
||||||
call_id,
|
|
||||||
output: FunctionCallOutputPayload {
|
|
||||||
content: format!("execution error: {e}"),
|
|
||||||
success: None,
|
|
||||||
},
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
_ => {
|
|
||||||
match try_parse_fully_qualified_tool_name(&name) {
|
|
||||||
Some((server, tool_name)) => {
|
|
||||||
// TODO(mbolin): Determine appropriate timeout for tool call.
|
|
||||||
let timeout = None;
|
|
||||||
handle_mcp_tool_call(
|
|
||||||
sess, &sub_id, call_id, server, tool_name, arguments, timeout,
|
|
||||||
)
|
|
||||||
.await
|
|
||||||
}
|
|
||||||
None => {
|
|
||||||
// Unknown function: reply with structured failure so the model can adapt.
|
|
||||||
ResponseInputItem::FunctionCallOutput {
|
|
||||||
call_id,
|
|
||||||
output: crate::models::FunctionCallOutputPayload {
|
|
||||||
content: format!("unsupported call: {}", name),
|
|
||||||
success: None,
|
|
||||||
},
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn apply_patch(
|
async fn apply_patch(
|
||||||
|
|||||||
Reference in New Issue
Block a user