Skip to content

Commit 64d3db8

Browse files
jif-oaiJeffCarpenter
jif-oai
authored and
JeffCarpenter
committed
chore: clean handle_container_exec_with_params (openai#5516)
Drop `handle_container_exec_with_params` to have simpler and more straight forward execution path
1 parent ecaeb60 commit 64d3db8

File tree

5 files changed

+343
-273
lines changed

5 files changed

+343
-273
lines changed

‎codex-rs/core/src/codex.rs‎

Lines changed: 48 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2316,7 +2316,11 @@ mod tests {
23162316
use crate::tools::MODEL_FORMAT_MAX_LINES;
23172317
use crate::tools::MODEL_FORMAT_TAIL_LINES;
23182318
use crate::tools::ToolRouter;
2319-
use crate::tools::handle_container_exec_with_params;
2319+
use crate::tools::context::ToolInvocation;
2320+
use crate::tools::context::ToolOutput;
2321+
use crate::tools::context::ToolPayload;
2322+
use crate::tools::handlers::ShellHandler;
2323+
use crate::tools::registry::ToolHandler;
23202324
use crate::turn_diff_tracker::TurnDiffTracker;
23212325
use codex_app_server_protocol::AuthMode;
23222326
use codex_protocol::models::ContentItem;
@@ -3039,15 +3043,26 @@ mod tests {
30393043
let tool_name = "shell";
30403044
let call_id = "test-call".to_string();
30413045

3042-
let resp = handle_container_exec_with_params(
3043-
tool_name,
3044-
params,
3045-
Arc::clone(&session),
3046-
Arc::clone(&turn_context),
3047-
Arc::clone(&turn_diff_tracker),
3048-
call_id,
3049-
)
3050-
.await;
3046+
let handler = ShellHandler;
3047+
let resp = handler
3048+
.handle(ToolInvocation {
3049+
session: Arc::clone(&session),
3050+
turn: Arc::clone(&turn_context),
3051+
tracker: Arc::clone(&turn_diff_tracker),
3052+
call_id,
3053+
tool_name: tool_name.to_string(),
3054+
payload: ToolPayload::Function {
3055+
arguments: serde_json::json!({
3056+
"command": params.command.clone(),
3057+
"workdir": Some(turn_context.cwd.to_string_lossy().to_string()),
3058+
"timeout_ms": params.timeout_ms,
3059+
"with_escalated_permissions": params.with_escalated_permissions,
3060+
"justification": params.justification.clone(),
3061+
})
3062+
.to_string(),
3063+
},
3064+
})
3065+
.await;
30513066

30523067
let Err(FunctionCallError::RespondToModel(output)) = resp else {
30533068
panic!("expected error result");
@@ -3066,17 +3081,30 @@ mod tests {
30663081
.expect("unique turn context Arc")
30673082
.sandbox_policy = SandboxPolicy::DangerFullAccess;
30683083

3069-
let resp2 = handle_container_exec_with_params(
3070-
tool_name,
3071-
params2,
3072-
Arc::clone(&session),
3073-
Arc::clone(&turn_context),
3074-
Arc::clone(&turn_diff_tracker),
3075-
"test-call-2".to_string(),
3076-
)
3077-
.await;
3084+
let resp2 = handler
3085+
.handle(ToolInvocation {
3086+
session: Arc::clone(&session),
3087+
turn: Arc::clone(&turn_context),
3088+
tracker: Arc::clone(&turn_diff_tracker),
3089+
call_id: "test-call-2".to_string(),
3090+
tool_name: tool_name.to_string(),
3091+
payload: ToolPayload::Function {
3092+
arguments: serde_json::json!({
3093+
"command": params2.command.clone(),
3094+
"workdir": Some(turn_context.cwd.to_string_lossy().to_string()),
3095+
"timeout_ms": params2.timeout_ms,
3096+
"with_escalated_permissions": params2.with_escalated_permissions,
3097+
"justification": params2.justification.clone(),
3098+
})
3099+
.to_string(),
3100+
},
3101+
})
3102+
.await;
30783103

3079-
let output = resp2.expect("expected Ok result");
3104+
let output = match resp2.expect("expected Ok result") {
3105+
ToolOutput::Function { content, .. } => content,
3106+
_ => panic!("unexpected tool output"),
3107+
};
30803108

30813109
#[derive(Deserialize, PartialEq, Eq, Debug)]
30823110
struct ResponseExecMetadata {

‎codex-rs/core/src/tools/events.rs‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
use crate::codex::Session;
22
use crate::codex::TurnContext;
3+
use crate::error::CodexErr;
4+
use crate::error::SandboxErr;
35
use crate::exec::ExecToolCallOutput;
6+
use crate::function_tool::FunctionCallError;
47
use crate::parse_command::parse_command;
58
use crate::protocol::EventMsg;
69
use crate::protocol::ExecCommandBeginEvent;
@@ -10,6 +13,7 @@ use crate::protocol::PatchApplyBeginEvent;
1013
use crate::protocol::PatchApplyEndEvent;
1114
use crate::protocol::TurnDiffEvent;
1215
use crate::tools::context::SharedTurnDiffTracker;
16+
use crate::tools::sandboxing::ToolError;
1317
use std::collections::HashMap;
1418
use std::path::Path;
1519
use std::path::PathBuf;
@@ -202,6 +206,56 @@ impl ToolEmitter {
202206
}
203207
}
204208
}
209+
210+
pub async fn begin(&self, ctx: ToolEventCtx<'_>) {
211+
self.emit(ctx, ToolEventStage::Begin).await;
212+
}
213+
214+
pub async fn finish(
215+
&self,
216+
ctx: ToolEventCtx<'_>,
217+
out: Result<ExecToolCallOutput, ToolError>,
218+
) -> Result<String, FunctionCallError> {
219+
let event;
220+
let result = match out {
221+
Ok(output) => {
222+
let content = super::format_exec_output_for_model(&output);
223+
let exit_code = output.exit_code;
224+
event = ToolEventStage::Success(output);
225+
if exit_code == 0 {
226+
Ok(content)
227+
} else {
228+
Err(FunctionCallError::RespondToModel(content))
229+
}
230+
}
231+
Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Timeout { output })))
232+
| Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output }))) => {
233+
let response = super::format_exec_output_for_model(&output);
234+
event = ToolEventStage::Failure(ToolEventFailure::Output(*output));
235+
Err(FunctionCallError::RespondToModel(response))
236+
}
237+
Err(ToolError::Codex(err)) => {
238+
let message = format!("execution error: {err:?}");
239+
let response = super::format_exec_output(&message);
240+
event = ToolEventStage::Failure(ToolEventFailure::Message(message));
241+
Err(FunctionCallError::RespondToModel(response))
242+
}
243+
Err(ToolError::Rejected(msg)) | Err(ToolError::SandboxDenied(msg)) => {
244+
// Normalize common rejection messages for exec tools so tests and
245+
// users see a clear, consistent phrase.
246+
let normalized = if msg == "rejected by user" {
247+
"exec command rejected by user".to_string()
248+
} else {
249+
msg
250+
};
251+
let response = super::format_exec_output(&normalized);
252+
event = ToolEventStage::Failure(ToolEventFailure::Message(normalized));
253+
Err(FunctionCallError::RespondToModel(response))
254+
}
255+
};
256+
self.emit(ctx, event).await;
257+
result
258+
}
205259
}
206260

207261
async fn emit_exec_end(

‎codex-rs/core/src/tools/handlers/apply_patch.rs‎

Lines changed: 86 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,24 @@
11
use std::collections::BTreeMap;
2-
use std::collections::HashMap;
3-
use std::sync::Arc;
42

3+
use crate::apply_patch;
4+
use crate::apply_patch::InternalApplyPatchInvocation;
5+
use crate::apply_patch::convert_apply_patch_to_protocol;
56
use crate::client_common::tools::FreeformTool;
67
use crate::client_common::tools::FreeformToolFormat;
78
use crate::client_common::tools::ResponsesApiTool;
89
use crate::client_common::tools::ToolSpec;
9-
use crate::exec::ExecParams;
1010
use crate::function_tool::FunctionCallError;
1111
use crate::tools::context::ToolInvocation;
1212
use crate::tools::context::ToolOutput;
1313
use crate::tools::context::ToolPayload;
14-
use crate::tools::handle_container_exec_with_params;
14+
use crate::tools::events::ToolEmitter;
15+
use crate::tools::events::ToolEventCtx;
16+
use crate::tools::orchestrator::ToolOrchestrator;
1517
use crate::tools::registry::ToolHandler;
1618
use crate::tools::registry::ToolKind;
19+
use crate::tools::runtimes::apply_patch::ApplyPatchRequest;
20+
use crate::tools::runtimes::apply_patch::ApplyPatchRuntime;
21+
use crate::tools::sandboxing::ToolCtx;
1722
use crate::tools::spec::ApplyPatchToolArgs;
1823
use crate::tools::spec::JsonSchema;
1924
use async_trait::async_trait;
@@ -64,30 +69,85 @@ impl ToolHandler for ApplyPatchHandler {
6469
}
6570
};
6671

67-
let exec_params = ExecParams {
68-
command: vec!["apply_patch".to_string(), patch_input.clone()],
69-
cwd: turn.cwd.clone(),
70-
timeout_ms: None,
71-
env: HashMap::new(),
72-
with_escalated_permissions: None,
73-
justification: None,
74-
arg0: None,
75-
};
72+
// Re-parse and verify the patch so we can compute changes and approval.
73+
// Avoid building temporary ExecParams/command vectors; derive directly from inputs.
74+
let cwd = turn.cwd.clone();
75+
let command = vec!["apply_patch".to_string(), patch_input.clone()];
76+
match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd) {
77+
codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => {
78+
match apply_patch::apply_patch(session.as_ref(), turn.as_ref(), &call_id, changes)
79+
.await
80+
{
81+
InternalApplyPatchInvocation::Output(item) => {
82+
let content = item?;
83+
Ok(ToolOutput::Function {
84+
content,
85+
success: Some(true),
86+
})
87+
}
88+
InternalApplyPatchInvocation::DelegateToExec(apply) => {
89+
let emitter = ToolEmitter::apply_patch(
90+
convert_apply_patch_to_protocol(&apply.action),
91+
!apply.user_explicitly_approved_this_action,
92+
);
93+
let event_ctx = ToolEventCtx::new(
94+
session.as_ref(),
95+
turn.as_ref(),
96+
&call_id,
97+
Some(&tracker),
98+
);
99+
emitter.begin(event_ctx).await;
76100

77-
let content = handle_container_exec_with_params(
78-
tool_name.as_str(),
79-
exec_params,
80-
Arc::clone(&session),
81-
Arc::clone(&turn),
82-
Arc::clone(&tracker),
83-
call_id.clone(),
84-
)
85-
.await?;
101+
let req = ApplyPatchRequest {
102+
patch: apply.action.patch.clone(),
103+
cwd,
104+
timeout_ms: None,
105+
user_explicitly_approved: apply.user_explicitly_approved_this_action,
106+
codex_exe: turn.codex_linux_sandbox_exe.clone(),
107+
};
86108

87-
Ok(ToolOutput::Function {
88-
content,
89-
success: Some(true),
90-
})
109+
let mut orchestrator = ToolOrchestrator::new();
110+
let mut runtime = ApplyPatchRuntime::new();
111+
let tool_ctx = ToolCtx {
112+
session: session.as_ref(),
113+
turn: turn.as_ref(),
114+
call_id: call_id.clone(),
115+
tool_name: tool_name.to_string(),
116+
};
117+
let out = orchestrator
118+
.run(&mut runtime, &req, &tool_ctx, &turn, turn.approval_policy)
119+
.await;
120+
let event_ctx = ToolEventCtx::new(
121+
session.as_ref(),
122+
turn.as_ref(),
123+
&call_id,
124+
Some(&tracker),
125+
);
126+
let content = emitter.finish(event_ctx, out).await?;
127+
Ok(ToolOutput::Function {
128+
content,
129+
success: Some(true),
130+
})
131+
}
132+
}
133+
}
134+
codex_apply_patch::MaybeApplyPatchVerified::CorrectnessError(parse_error) => {
135+
Err(FunctionCallError::RespondToModel(format!(
136+
"apply_patch verification failed: {parse_error}"
137+
)))
138+
}
139+
codex_apply_patch::MaybeApplyPatchVerified::ShellParseError(error) => {
140+
tracing::trace!("Failed to parse apply_patch input, {error:?}");
141+
Err(FunctionCallError::RespondToModel(
142+
"apply_patch handler received invalid patch input".to_string(),
143+
))
144+
}
145+
codex_apply_patch::MaybeApplyPatchVerified::NotApplyPatch => {
146+
Err(FunctionCallError::RespondToModel(
147+
"apply_patch handler received non-apply_patch input".to_string(),
148+
))
149+
}
150+
}
91151
}
92152
}
93153

0 commit comments

Comments
 (0)