mirror of
https://github.com/openai/codex.git
synced 2026-10-22 17:58:46 +00:00
Standardize shell execution on unified exec (#39772)
## What changed - Use `exec_command` and `write_stdin` as the shell tool surface. - Treat legacy `default`, `local`, and `shell_command` model metadata as `unified_exec`. - Remove obsolete shell-selection configuration and runtime paths while preserving the feature and policy gates for zsh fork execution. GitOrigin-RevId: d743cbe598630d73052f1fecad680c4cde17977d
This commit is contained in:
@@ -112,7 +112,6 @@ impl ToolCallRuntime {
|
||||
let router = &self.step_context.tool_router;
|
||||
let supports_parallel = router.tool_supports_parallel(&call);
|
||||
let tool_runtime = router.tool_runtime(&call);
|
||||
let wait_for_runtime_cancellation = router.tool_waits_for_runtime_cancellation(&call);
|
||||
let router = Arc::clone(router);
|
||||
let session = Arc::clone(&self.session);
|
||||
let step_context = Arc::clone(&self.step_context);
|
||||
@@ -185,24 +184,11 @@ impl ToolCallRuntime {
|
||||
} else {
|
||||
let secs = started.elapsed().as_secs_f32().max(0.1);
|
||||
abort_dispatch_span.record("aborted", true);
|
||||
if wait_for_runtime_cancellation {
|
||||
if terminal_outcome_reached.swap(true, Ordering::AcqRel) {
|
||||
return dispatch_handle.await.map_err(Self::tool_task_join_error)?;
|
||||
}
|
||||
// The abort owns the terminal outcome; await only so
|
||||
// the runtime can finish process teardown.
|
||||
match dispatch_handle.await {
|
||||
Ok(_) => {}
|
||||
Err(err) if err.is_cancelled() => {}
|
||||
Err(err) => return Err(Self::tool_task_join_error(err)),
|
||||
}
|
||||
} else {
|
||||
dispatch_handle.abort();
|
||||
match dispatch_handle.await {
|
||||
Ok(result) => return result,
|
||||
Err(err) if err.is_cancelled() => {}
|
||||
Err(err) => return Err(Self::tool_task_join_error(err)),
|
||||
}
|
||||
dispatch_handle.abort();
|
||||
match dispatch_handle.await {
|
||||
Ok(result) => return result,
|
||||
Err(err) if err.is_cancelled() => {}
|
||||
Err(err) => return Err(Self::tool_task_join_error(err)),
|
||||
}
|
||||
let response = Self::aborted_response(&call, secs);
|
||||
notify_tool_aborted(
|
||||
@@ -266,12 +252,7 @@ impl ToolCallRuntime {
|
||||
}
|
||||
|
||||
fn abort_message(call: &ToolCall, secs: f32) -> String {
|
||||
if call.tool_name.is_default_namespace()
|
||||
&& matches!(
|
||||
call.tool_name.name.as_str(),
|
||||
"exec_command" | "unified_exec"
|
||||
)
|
||||
{
|
||||
if call.tool_name.is_default_namespace() && call.tool_name.name == "exec_command" {
|
||||
format!("Wall time: {secs:.1} seconds\naborted by user")
|
||||
} else {
|
||||
format!("aborted by user after {secs:.1}s")
|
||||
@@ -568,90 +549,6 @@ mod tests {
|
||||
|
||||
impl CoreToolRuntime for ImmediateHandler {}
|
||||
|
||||
struct CancellationCleanupHandler {
|
||||
tool_name: codex_tools::ToolName,
|
||||
started: std::sync::Mutex<Option<oneshot::Sender<()>>>,
|
||||
cleanup_started: std::sync::Mutex<Option<oneshot::Sender<()>>>,
|
||||
allow_cleanup: Arc<Notify>,
|
||||
}
|
||||
|
||||
impl ToolExecutor<ToolInvocation> for CancellationCleanupHandler {
|
||||
fn tool_name(&self) -> codex_tools::ToolName {
|
||||
self.tool_name.clone()
|
||||
}
|
||||
|
||||
fn spec(&self) -> codex_tools::ToolSpec {
|
||||
codex_tools::ToolSpec::Function(codex_tools::ResponsesApiTool {
|
||||
name: self.tool_name.name.clone(),
|
||||
description: "Cancellation cleanup test tool.".to_string(),
|
||||
strict: false,
|
||||
defer_loading: None,
|
||||
parameters: codex_tools::JsonSchema::default(),
|
||||
output_schema: None,
|
||||
})
|
||||
}
|
||||
|
||||
fn handle(&self, invocation: ToolInvocation) -> codex_tools::ToolExecutorFuture<'_> {
|
||||
Box::pin(self.handle_call(invocation))
|
||||
}
|
||||
}
|
||||
|
||||
impl CancellationCleanupHandler {
|
||||
async fn handle_call(
|
||||
&self,
|
||||
invocation: ToolInvocation,
|
||||
) -> Result<Box<dyn crate::tools::context::ToolOutput>, FunctionCallError> {
|
||||
let started = self
|
||||
.started
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.take();
|
||||
if let Some(started) = started {
|
||||
let _ = started.send(());
|
||||
}
|
||||
invocation.cancellation_token.cancelled().await;
|
||||
let cleanup_started = self
|
||||
.cleanup_started
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.take();
|
||||
if let Some(cleanup_started) = cleanup_started {
|
||||
let _ = cleanup_started.send(());
|
||||
}
|
||||
self.allow_cleanup.notified().await;
|
||||
Ok(Box::new(FunctionToolOutput::from_text(
|
||||
"cleanup complete".to_string(),
|
||||
Some(false),
|
||||
)) as Box<dyn crate::tools::context::ToolOutput>)
|
||||
}
|
||||
}
|
||||
|
||||
impl CoreToolRuntime for CancellationCleanupHandler {
|
||||
fn waits_for_runtime_cancellation(&self) -> bool {
|
||||
true
|
||||
}
|
||||
}
|
||||
|
||||
struct FinishRecorder {
|
||||
records: Arc<std::sync::Mutex<Vec<ToolCallOutcome>>>,
|
||||
}
|
||||
|
||||
impl codex_extension_api::ToolLifecycleContributor for FinishRecorder {
|
||||
fn on_tool_finish<'a>(
|
||||
&'a self,
|
||||
input: codex_extension_api::ToolFinishInput<'a>,
|
||||
) -> codex_extension_api::ToolLifecycleFuture<'a> {
|
||||
let records = Arc::clone(&self.records);
|
||||
let outcome = input.outcome;
|
||||
Box::pin(async move {
|
||||
records
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.push(outcome);
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
struct BlockingFinishContributor {
|
||||
records: Arc<std::sync::Mutex<Vec<ToolCallOutcome>>>,
|
||||
finish_started: std::sync::Mutex<Option<oneshot::Sender<()>>>,
|
||||
@@ -756,78 +653,4 @@ mod tests {
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn cancellation_waiting_for_runtime_cleanup_emits_only_aborted_lifecycle()
|
||||
-> anyhow::Result<()> {
|
||||
let (mut session, turn_context) = crate::session::tests::make_session_and_context().await;
|
||||
let records = Arc::new(std::sync::Mutex::new(Vec::new()));
|
||||
let mut builder =
|
||||
codex_extension_api::ExtensionRegistryBuilder::<crate::config::Config>::new();
|
||||
builder.tool_lifecycle_contributor(Arc::new(FinishRecorder {
|
||||
records: Arc::clone(&records),
|
||||
}));
|
||||
session.services.extensions = Arc::new(builder.build());
|
||||
|
||||
let session = Arc::new(session);
|
||||
let turn_context = Arc::new(turn_context);
|
||||
let tool_name = codex_tools::ToolName::plain("cleanup_tool");
|
||||
let (started_tx, started_rx) = oneshot::channel();
|
||||
let (cleanup_started_tx, cleanup_started_rx) = oneshot::channel();
|
||||
let allow_cleanup = Arc::new(Notify::new());
|
||||
let handler = Arc::new(CancellationCleanupHandler {
|
||||
tool_name: tool_name.clone(),
|
||||
started: std::sync::Mutex::new(Some(started_tx)),
|
||||
cleanup_started: std::sync::Mutex::new(Some(cleanup_started_tx)),
|
||||
allow_cleanup: Arc::clone(&allow_cleanup),
|
||||
}) as Arc<dyn CoreToolRuntime>;
|
||||
let step_context = StepContext::for_test(Arc::clone(&turn_context));
|
||||
let router = Arc::new(ToolRouter::from_parts(
|
||||
ToolRegistry::from_tools([handler]),
|
||||
Vec::new(),
|
||||
));
|
||||
let step_context = step_context.with_tool_router_for_test(router);
|
||||
let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new()));
|
||||
let runtime = ToolCallRuntime::new(session, step_context, tracker);
|
||||
let cancellation_token = CancellationToken::new();
|
||||
let call = ToolCall {
|
||||
tool_name,
|
||||
call_id: "call-1".to_string(),
|
||||
payload: ToolPayload::Function {
|
||||
arguments: "{}".to_string(),
|
||||
},
|
||||
encrypted_function_args: None,
|
||||
};
|
||||
|
||||
let response_task =
|
||||
tokio::spawn(runtime.handle_tool_call(call, cancellation_token.clone()));
|
||||
started_rx.await.expect("handler should start");
|
||||
cancellation_token.cancel();
|
||||
cleanup_started_rx
|
||||
.await
|
||||
.expect("handler should start cleanup");
|
||||
tokio::time::sleep(Duration::from_millis(10)).await;
|
||||
allow_cleanup.notify_one();
|
||||
|
||||
let response = tokio::time::timeout(Duration::from_secs(1), response_task)
|
||||
.await
|
||||
.expect("timed out waiting for tool response")
|
||||
.expect("tool response task should join")?;
|
||||
let ResponseInputItem::FunctionCallOutput { output, .. } = response else {
|
||||
anyhow::bail!("cancelled tool should return function output");
|
||||
};
|
||||
let FunctionCallOutputBody::Text(text) = output.body else {
|
||||
anyhow::bail!("cancelled tool output should be text");
|
||||
};
|
||||
assert!(text.contains("aborted by user"));
|
||||
|
||||
let actual = records
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.drain(..)
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(vec![ToolCallOutcome::Aborted], actual);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user