mirror of
https://github.com/openai/codex.git
synced 2026-11-01 00:15:56 +00:00
feat: adding piped process to replace PTY when needed (#8797)
This commit is contained in:
@@ -32,6 +32,7 @@ use crate::sandboxing::SandboxPermissions;
|
||||
use crate::spawn::StdioPolicy;
|
||||
use crate::spawn::spawn_child_async;
|
||||
use crate::text_encoding::bytes_to_string_smart;
|
||||
use codex_utils_pty::process_group::kill_child_process_group;
|
||||
|
||||
pub const DEFAULT_EXEC_COMMAND_TIMEOUT_MS: u64 = 10_000;
|
||||
|
||||
@@ -750,38 +751,6 @@ fn synthetic_exit_status(code: i32) -> ExitStatus {
|
||||
std::process::ExitStatus::from_raw(code as u32)
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
fn kill_child_process_group(child: &mut Child) -> io::Result<()> {
|
||||
use std::io::ErrorKind;
|
||||
|
||||
if let Some(pid) = child.id() {
|
||||
let pid = pid as libc::pid_t;
|
||||
let pgid = unsafe { libc::getpgid(pid) };
|
||||
if pgid == -1 {
|
||||
let err = std::io::Error::last_os_error();
|
||||
if err.kind() != ErrorKind::NotFound {
|
||||
return Err(err);
|
||||
}
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let result = unsafe { libc::killpg(pgid, libc::SIGKILL) };
|
||||
if result == -1 {
|
||||
let err = std::io::Error::last_os_error();
|
||||
if err.kind() != ErrorKind::NotFound {
|
||||
return Err(err);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(not(unix))]
|
||||
fn kill_child_process_group(_: &mut Child) -> io::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
@@ -70,8 +70,8 @@ pub(crate) async fn spawn_child_async(
|
||||
#[cfg(target_os = "linux")]
|
||||
let parent_pid = libc::getpid();
|
||||
cmd.pre_exec(move || {
|
||||
if set_process_group && libc::setpgid(0, 0) == -1 {
|
||||
return Err(std::io::Error::last_os_error());
|
||||
if set_process_group {
|
||||
codex_utils_pty::process_group::set_process_group()?;
|
||||
}
|
||||
|
||||
// This relies on prctl(2), so it only works on Linux.
|
||||
@@ -79,18 +79,7 @@ pub(crate) async fn spawn_child_async(
|
||||
{
|
||||
// This prctl call effectively requests, "deliver SIGTERM when my
|
||||
// current parent dies."
|
||||
if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM) == -1 {
|
||||
return Err(std::io::Error::last_os_error());
|
||||
}
|
||||
|
||||
// Though if there was a race condition and this pre_exec() block is
|
||||
// run _after_ the parent (i.e., the Codex process) has already
|
||||
// exited, then parent will be the closest configured "subreaper"
|
||||
// ancestor process, or PID 1 (init). If the Codex process has exited
|
||||
// already, so should the child process.
|
||||
if libc::getppid() != parent_pid {
|
||||
libc::raise(libc::SIGTERM);
|
||||
}
|
||||
codex_utils_pty::process_group::set_parent_death_signal(parent_pid)?;
|
||||
}
|
||||
Ok(())
|
||||
});
|
||||
|
||||
@@ -33,6 +33,8 @@ struct ExecCommandArgs {
|
||||
shell: Option<String>,
|
||||
#[serde(default = "default_login")]
|
||||
login: bool,
|
||||
#[serde(default = "default_tty")]
|
||||
tty: bool,
|
||||
#[serde(default = "default_exec_yield_time_ms")]
|
||||
yield_time_ms: u64,
|
||||
#[serde(default)]
|
||||
@@ -67,6 +69,10 @@ fn default_login() -> bool {
|
||||
true
|
||||
}
|
||||
|
||||
fn default_tty() -> bool {
|
||||
false
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl ToolHandler for UnifiedExecHandler {
|
||||
fn kind(&self) -> ToolKind {
|
||||
@@ -124,6 +130,7 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
|
||||
let ExecCommandArgs {
|
||||
workdir,
|
||||
tty,
|
||||
yield_time_ms,
|
||||
max_output_tokens,
|
||||
sandbox_permissions,
|
||||
@@ -173,6 +180,7 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
yield_time_ms,
|
||||
max_output_tokens,
|
||||
workdir,
|
||||
tty,
|
||||
sandbox_permissions,
|
||||
justification,
|
||||
},
|
||||
|
||||
@@ -37,6 +37,7 @@ pub struct UnifiedExecRequest {
|
||||
pub command: Vec<String>,
|
||||
pub cwd: PathBuf,
|
||||
pub env: HashMap<String, String>,
|
||||
pub tty: bool,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub justification: Option<String>,
|
||||
pub exec_approval_requirement: ExecApprovalRequirement,
|
||||
@@ -46,6 +47,7 @@ pub struct UnifiedExecRequest {
|
||||
pub struct UnifiedExecApprovalKey {
|
||||
pub command: Vec<String>,
|
||||
pub cwd: PathBuf,
|
||||
pub tty: bool,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
}
|
||||
|
||||
@@ -58,6 +60,7 @@ impl UnifiedExecRequest {
|
||||
command: Vec<String>,
|
||||
cwd: PathBuf,
|
||||
env: HashMap<String, String>,
|
||||
tty: bool,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
justification: Option<String>,
|
||||
exec_approval_requirement: ExecApprovalRequirement,
|
||||
@@ -66,6 +69,7 @@ impl UnifiedExecRequest {
|
||||
command,
|
||||
cwd,
|
||||
env,
|
||||
tty,
|
||||
sandbox_permissions,
|
||||
justification,
|
||||
exec_approval_requirement,
|
||||
@@ -96,6 +100,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
vec![UnifiedExecApprovalKey {
|
||||
command: req.command.clone(),
|
||||
cwd: req.cwd.clone(),
|
||||
tty: req.tty,
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
}]
|
||||
}
|
||||
@@ -189,7 +194,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
.env_for(spec)
|
||||
.map_err(|err| ToolError::Codex(err.into()))?;
|
||||
self.manager
|
||||
.open_session_with_exec_env(&exec_env)
|
||||
.open_session_with_exec_env(&exec_env, req.tty)
|
||||
.await
|
||||
.map_err(|err| match err {
|
||||
UnifiedExecError::SandboxDenied { output, .. } => {
|
||||
|
||||
@@ -168,6 +168,15 @@ fn create_exec_command_tool() -> ToolSpec {
|
||||
),
|
||||
},
|
||||
),
|
||||
(
|
||||
"tty".to_string(),
|
||||
JsonSchema::Boolean {
|
||||
description: Some(
|
||||
"Whether to allocate a TTY for the command. Defaults to false (plain pipes); set to true to open a PTY and access TTY process."
|
||||
.to_string(),
|
||||
),
|
||||
}
|
||||
),
|
||||
(
|
||||
"yield_time_ms".to_string(),
|
||||
JsonSchema::Number {
|
||||
|
||||
@@ -79,6 +79,7 @@ pub(crate) struct ExecCommandRequest {
|
||||
pub yield_time_ms: u64,
|
||||
pub max_output_tokens: Option<usize>,
|
||||
pub workdir: Option<PathBuf>,
|
||||
pub tty: bool,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub justification: Option<String>,
|
||||
}
|
||||
@@ -200,6 +201,7 @@ mod tests {
|
||||
yield_time_ms,
|
||||
max_output_tokens: None,
|
||||
workdir: None,
|
||||
tty: true,
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
justification: None,
|
||||
},
|
||||
|
||||
@@ -126,6 +126,7 @@ impl UnifiedExecProcessManager {
|
||||
cwd.clone(),
|
||||
request.sandbox_permissions,
|
||||
request.justification,
|
||||
request.tty,
|
||||
context,
|
||||
)
|
||||
.await;
|
||||
@@ -442,21 +443,34 @@ impl UnifiedExecProcessManager {
|
||||
pub(crate) async fn open_session_with_exec_env(
|
||||
&self,
|
||||
env: &ExecEnv,
|
||||
tty: bool,
|
||||
) -> Result<UnifiedExecProcess, UnifiedExecError> {
|
||||
let (program, args) = env
|
||||
.command
|
||||
.split_first()
|
||||
.ok_or(UnifiedExecError::MissingCommandLine)?;
|
||||
|
||||
let spawned = codex_utils_pty::spawn_pty_process(
|
||||
program,
|
||||
args,
|
||||
env.cwd.as_path(),
|
||||
&env.env,
|
||||
&env.arg0,
|
||||
)
|
||||
.await
|
||||
.map_err(|err| UnifiedExecError::create_process(err.to_string()))?;
|
||||
let spawn_result = if tty {
|
||||
codex_utils_pty::pty::spawn_process(
|
||||
program,
|
||||
args,
|
||||
env.cwd.as_path(),
|
||||
&env.env,
|
||||
&env.arg0,
|
||||
)
|
||||
.await
|
||||
} else {
|
||||
codex_utils_pty::pipe::spawn_process(
|
||||
program,
|
||||
args,
|
||||
env.cwd.as_path(),
|
||||
&env.env,
|
||||
&env.arg0,
|
||||
)
|
||||
.await
|
||||
};
|
||||
let spawned =
|
||||
spawn_result.map_err(|err| UnifiedExecError::create_process(err.to_string()))?;
|
||||
UnifiedExecProcess::from_spawned(spawned, env.sandbox).await
|
||||
}
|
||||
|
||||
@@ -466,6 +480,7 @@ impl UnifiedExecProcessManager {
|
||||
cwd: PathBuf,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
justification: Option<String>,
|
||||
tty: bool,
|
||||
context: &UnifiedExecContext,
|
||||
) -> Result<UnifiedExecProcess, UnifiedExecError> {
|
||||
let env = apply_unified_exec_env(create_env(&context.turn.shell_environment_policy));
|
||||
@@ -488,6 +503,7 @@ impl UnifiedExecProcessManager {
|
||||
command.to_vec(),
|
||||
cwd,
|
||||
env,
|
||||
tty,
|
||||
sandbox_permissions,
|
||||
justification,
|
||||
exec_approval_requirement,
|
||||
|
||||
Reference in New Issue
Block a user