Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions codex-rs/cli/src/debug_sandbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ async fn run_command_under_sandbox(
command_vec,
&cwd_clone,
env_map,
/*stdin_bytes*/ None,
/*timeout_ms*/ None,
config.permissions.windows_sandbox_private_desktop,
)
Expand All @@ -181,6 +182,7 @@ async fn run_command_under_sandbox(
command_vec,
&cwd_clone,
env_map,
/*stdin_bytes*/ None,
/*timeout_ms*/ None,
config.permissions.windows_sandbox_private_desktop,
)
Expand Down
2 changes: 2 additions & 0 deletions codex-rs/core/src/codex_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4798,6 +4798,7 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() {
capture_policy: ExecCapturePolicy::ShellTool,
env: HashMap::new(),
network: None,
stdin: crate::exec::ExecStdin::Closed,
sandbox_permissions,
windows_sandbox_level: turn_context.windows_sandbox_level,
windows_sandbox_private_desktop: turn_context
Expand All @@ -4816,6 +4817,7 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() {
capture_policy: ExecCapturePolicy::ShellTool,
env: HashMap::new(),
network: None,
stdin: crate::exec::ExecStdin::Closed,
windows_sandbox_level: turn_context.windows_sandbox_level,
windows_sandbox_private_desktop: turn_context
.config
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/src/codex_tests_guardian.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid
capture_policy: ExecCapturePolicy::ShellTool,
env: HashMap::new(),
network: None,
stdin: crate::exec::ExecStdin::Closed,
sandbox_permissions: SandboxPermissions::WithAdditionalPermissions,
windows_sandbox_level: turn_context.windows_sandbox_level,
windows_sandbox_private_desktop: turn_context
Expand Down
41 changes: 39 additions & 2 deletions codex-rs/core/src/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use std::time::Instant;
use async_channel::Sender;
use tokio::io::AsyncRead;
use tokio::io::AsyncReadExt;
use tokio::io::AsyncWriteExt;
use tokio::io::BufReader;
use tokio::process::Child;
use tokio_util::sync::CancellationToken;
Expand Down Expand Up @@ -81,6 +82,7 @@ pub struct ExecParams {
pub capture_policy: ExecCapturePolicy,
pub env: HashMap<String, String>,
pub network: Option<NetworkProxy>,
pub stdin: ExecStdin,
pub sandbox_permissions: SandboxPermissions,
pub windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel,
pub windows_sandbox_private_desktop: bool,
Expand All @@ -97,7 +99,12 @@ pub enum ExecCapturePolicy {
/// without the shell-oriented output cap or exec-expiration behavior.
FullBuffer,
}

#[derive(Clone, Debug, Default)]
pub enum ExecStdin {
#[default]
Closed,
Bytes(Vec<u8>),
}
fn select_process_exec_tool_sandbox_type(
file_system_sandbox_policy: &FileSystemSandboxPolicy,
network_sandbox_policy: NetworkSandboxPolicy,
Expand Down Expand Up @@ -263,6 +270,7 @@ pub fn build_exec_request(
expiration,
capture_policy,
network,
stdin: _stdin,
sandbox_permissions,
windows_sandbox_level,
windows_sandbox_private_desktop,
Expand Down Expand Up @@ -380,6 +388,7 @@ fn prepare_exec_request(exec_request: ExecRequest) -> PreparedExecRequest {
cwd,
env,
network,
stdin,
expiration,
capture_policy,
sandbox,
Expand All @@ -402,6 +411,7 @@ fn prepare_exec_request(exec_request: ExecRequest) -> PreparedExecRequest {
capture_policy,
env,
network,
stdin,
sandbox_permissions,
windows_sandbox_level,
windows_sandbox_private_desktop,
Expand Down Expand Up @@ -495,6 +505,7 @@ async fn exec_windows_sandbox(
cwd,
mut env,
network,
stdin,
expiration,
capture_policy,
windows_sandbox_level,
Expand Down Expand Up @@ -536,6 +547,10 @@ async fn exec_windows_sandbox(
command,
&cwd,
env,
match stdin {
ExecStdin::Closed => None,
ExecStdin::Bytes(bytes) => Some(bytes),
},
timeout_ms,
windows_sandbox_private_desktop,
)
Expand All @@ -547,6 +562,10 @@ async fn exec_windows_sandbox(
command,
&cwd,
env,
match stdin {
ExecStdin::Closed => None,
ExecStdin::Bytes(bytes) => Some(bytes),
},
timeout_ms,
windows_sandbox_private_desktop,
)
Expand Down Expand Up @@ -913,6 +932,7 @@ async fn exec(
mut env,
network,
arg0,
stdin,
expiration,
capture_policy,
windows_sandbox_level: _,
Expand Down Expand Up @@ -941,12 +961,13 @@ async fn exec(
network: None,
stdio_policy: StdioPolicy::RedirectForShellTool,
env,
stdin_open: matches!(stdin, ExecStdin::Bytes(_)),
})
.await?;
if let Some(after_spawn) = after_spawn {
after_spawn();
}
consume_output(child, expiration, capture_policy, stdout_stream).await
consume_output(child, stdin, expiration, capture_policy, stdout_stream).await
}

#[cfg_attr(not(target_os = "windows"), allow(dead_code))]
Expand Down Expand Up @@ -1004,10 +1025,19 @@ fn windows_restricted_token_sandbox_support(
/// policy.
async fn consume_output(
mut child: Child,
stdin: ExecStdin,
expiration: ExecExpiration,
capture_policy: ExecCapturePolicy,
stdout_stream: Option<StdoutStream>,
) -> Result<RawExecToolCallOutput> {
let stdin_task = match (child.stdin.take(), stdin) {
(Some(mut child_stdin), ExecStdin::Bytes(bytes)) => Some(tokio::spawn(async move {
child_stdin.write_all(&bytes).await?;
child_stdin.shutdown().await
})),
_ => None,
};

// Both stdout and stderr were configured with `Stdio::piped()`
// above, therefore `take()` should normally return `Some`. If it doesn't
// we treat it as an exceptional I/O error
Expand Down Expand Up @@ -1090,6 +1120,13 @@ async fn consume_output(

let stdout = await_output(&mut stdout_handle, capture_policy.io_drain_timeout()).await?;
let stderr = await_output(&mut stderr_handle, capture_policy.io_drain_timeout()).await?;
if let Some(stdin_task) = stdin_task {
match stdin_task.await {
Ok(Ok(())) => {}
Ok(Err(err)) => return Err(CodexErr::Io(err)),
Err(join_err) => return Err(CodexErr::Io(io::Error::other(join_err))),
Comment on lines +1123 to +1127
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Don't fail exec when the child closes stdin early

When ExecStdin::Bytes is used with the direct exec() path, commands that stop reading before draining all input (for example head -c 1, parsers that only consume a header, or a child that exits immediately) now come back as I/O errors even if the process itself exits successfully. In that case child_stdin.write_all() returns BrokenPipe, and this block converts it into CodexErr::Io after we've already collected the child's exit status, so partial stdin consumers become unusable through the new API.

Useful? React with 👍 / 👎.

}
}
let aggregated_output = aggregate_output(&stdout, &stderr, retained_bytes_cap);

Ok(RawExecToolCallOutput {
Expand Down
52 changes: 52 additions & 0 deletions codex-rs/core/src/exec_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,56 @@ async fn read_output_limits_retained_bytes_for_shell_capture() {
assert_eq!(out.text.len(), EXEC_OUTPUT_MAX_BYTES);
}

#[tokio::test]
async fn exec_passes_stdin_bytes_to_child() -> Result<()> {
let command = if cfg!(windows) {
vec![
"cmd.exe".to_string(),
"/Q".to_string(),
"/D".to_string(),
"/C".to_string(),
"more".to_string(),
]
} else {
vec!["/bin/cat".to_string()]
};
let params = ExecParams {
command,
cwd: std::env::current_dir()?,
expiration: 1_000.into(),
env: std::env::vars().collect(),
network: None,
stdin: ExecStdin::Bytes(b"hello from stdin\n".to_vec()),
sandbox_permissions: SandboxPermissions::UseDefault,
windows_sandbox_level: WindowsSandboxLevel::Disabled,
windows_sandbox_private_desktop: false,
justification: None,
arg0: None,
};

let output = exec(
params,
SandboxType::None,
&SandboxPolicy::DangerFullAccess,
&FileSystemSandboxPolicy::from(&SandboxPolicy::DangerFullAccess),
NetworkSandboxPolicy::Enabled,
None,
None,
)
.await?;

let expected_stdout = if cfg!(windows) {
"hello from stdin\r\n"
} else {
"hello from stdin\n"
};
assert_eq!(output.exit_status.code(), Some(0));
assert_eq!(output.stdout.from_utf8_lossy().text, expected_stdout);
assert_eq!(output.stderr.from_utf8_lossy().text, "");

Ok(())
}

#[test]
fn aggregate_output_prefers_stderr_on_contention() {
let stdout = StreamOutput {
Expand Down Expand Up @@ -588,6 +638,7 @@ async fn kill_child_process_group_kills_grandchildren_on_timeout() -> Result<()>
capture_policy: ExecCapturePolicy::ShellTool,
env,
network: None,
stdin: ExecStdin::Closed,
sandbox_permissions: SandboxPermissions::UseDefault,
windows_sandbox_level: WindowsSandboxLevel::Disabled,
windows_sandbox_private_desktop: false,
Expand Down Expand Up @@ -646,6 +697,7 @@ async fn process_exec_tool_call_respects_cancellation_token() -> Result<()> {
capture_policy: ExecCapturePolicy::ShellTool,
env,
network: None,
stdin: ExecStdin::Closed,
sandbox_permissions: SandboxPermissions::UseDefault,
windows_sandbox_level: WindowsSandboxLevel::Disabled,
windows_sandbox_private_desktop: false,
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/src/landlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ where
network,
stdio_policy,
env,
stdin_open: false,
})
.await
}
Expand Down
2 changes: 2 additions & 0 deletions codex-rs/core/src/sandboxed_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::codex::Session;
use crate::codex::TurnContext;
use crate::exec::ExecCapturePolicy;
use crate::exec::ExecExpiration;
use crate::exec::ExecStdin;
use crate::exec::ExecToolCallRawOutput;
use crate::exec::execute_exec_request_raw_output;
use crate::sandboxing::CommandSpec;
Expand Down Expand Up @@ -104,6 +105,7 @@ async fn perform_operation(
exit_code: -1,
message: error.to_string(),
})?;
exec_request.stdin = stdin;

let effective_policy = exec_request.sandbox_policy.clone();
let output = execute_exec_request_raw_output(
Expand Down
3 changes: 3 additions & 0 deletions codex-rs/core/src/sandboxing/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub(crate) mod macos_permissions;

use crate::exec::ExecCapturePolicy;
use crate::exec::ExecExpiration;
use crate::exec::ExecStdin;
use crate::exec::ExecToolCallOutput;
use crate::exec::SandboxType;
use crate::exec::StdoutStream;
Expand Down Expand Up @@ -68,6 +69,7 @@ pub struct ExecRequest {
pub cwd: PathBuf,
pub env: HashMap<String, String>,
pub network: Option<NetworkProxy>,
pub stdin: ExecStdin,
pub expiration: ExecExpiration,
pub capture_policy: ExecCapturePolicy,
pub sandbox: SandboxType,
Expand Down Expand Up @@ -709,6 +711,7 @@ impl SandboxManager {
cwd: spec.cwd,
env,
network: network.cloned(),
stdin: ExecStdin::Closed,
expiration: spec.expiration,
capture_policy: spec.capture_policy,
sandbox,
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/src/seatbelt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ pub async fn spawn_command_under_seatbelt(
network,
stdio_policy,
env,
stdin_open: false,
})
.await
}
Expand Down
16 changes: 11 additions & 5 deletions codex-rs/core/src/spawn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ pub(crate) struct SpawnChildRequest<'a> {
pub network: Option<&'a NetworkProxy>,
pub stdio_policy: StdioPolicy,
pub env: HashMap<String, String>,
pub stdin_open: bool,
}

pub(crate) async fn spawn_child_async(request: SpawnChildRequest<'_>) -> std::io::Result<Child> {
Expand All @@ -57,6 +58,7 @@ pub(crate) async fn spawn_child_async(request: SpawnChildRequest<'_>) -> std::io
network,
stdio_policy,
mut env,
stdin_open,
} = request;

trace!(
Expand Down Expand Up @@ -105,11 +107,15 @@ pub(crate) async fn spawn_child_async(request: SpawnChildRequest<'_>) -> std::io

match stdio_policy {
StdioPolicy::RedirectForShellTool => {
// Do not create a file descriptor for stdin because otherwise some
// commands may hang forever waiting for input. For example, ripgrep has
// a heuristic where it may try to read from stdin as explained here:
// https://github.com/BurntSushi/ripgrep/blob/e2362d4d5185d02fa857bf381e7bd52e66fafc73/crates/core/flags/hiargs.rs#L1101-L1103
cmd.stdin(Stdio::null());
if stdin_open {
cmd.stdin(Stdio::piped());
} else {
// Do not create a file descriptor for stdin because otherwise some
// commands may hang forever waiting for input. For example, ripgrep has
// a heuristic where it may try to read from stdin as explained here:
// https://github.com/BurntSushi/ripgrep/blob/e2362d4d5185d02fa857bf381e7bd52e66fafc73/crates/core/flags/hiargs.rs#L1101-L1103
cmd.stdin(Stdio::null());
}

cmd.stdout(Stdio::piped()).stderr(Stdio::piped());
}
Expand Down
2 changes: 2 additions & 0 deletions codex-rs/core/src/tasks/user_shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use uuid::Uuid;

use crate::codex::TurnContext;
use crate::exec::ExecCapturePolicy;
use crate::exec::ExecStdin;
use crate::exec::ExecToolCallOutput;
use crate::exec::SandboxType;
use crate::exec::StdoutStream;
Expand Down Expand Up @@ -163,6 +164,7 @@ pub(crate) async fn execute_user_shell_command(
Some(session.conversation_id),
),
network: turn_context.network.clone(),
stdin: ExecStdin::Closed,
// TODO(zhao-oai): Now that we have ExecExpiration::Cancellation, we
// should use that instead of an "arbitrarily large" timeout here.
expiration: USER_SHELL_TIMEOUT_MS.into(),
Expand Down
3 changes: 3 additions & 0 deletions codex-rs/core/src/tools/handlers/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use std::sync::Arc;
use crate::codex::TurnContext;
use crate::exec::ExecCapturePolicy;
use crate::exec::ExecParams;
use crate::exec::ExecStdin;
use crate::exec_env::create_env;
use crate::exec_policy::ExecApprovalRequest;
use crate::function_tool::FunctionCallError;
Expand Down Expand Up @@ -74,6 +75,7 @@ impl ShellHandler {
capture_policy: ExecCapturePolicy::ShellTool,
env: create_env(&turn_context.shell_environment_policy, Some(thread_id)),
network: turn_context.network.clone(),
stdin: ExecStdin::Closed,
sandbox_permissions: params.sandbox_permissions.unwrap_or_default(),
windows_sandbox_level: turn_context.windows_sandbox_level,
windows_sandbox_private_desktop: turn_context
Expand Down Expand Up @@ -129,6 +131,7 @@ impl ShellCommandHandler {
capture_policy: ExecCapturePolicy::ShellTool,
env: create_env(&turn_context.shell_environment_policy, Some(thread_id)),
network: turn_context.network.clone(),
stdin: ExecStdin::Closed,
sandbox_permissions: params.sandbox_permissions.unwrap_or_default(),
windows_sandbox_level: turn_context.windows_sandbox_level,
windows_sandbox_private_desktop: turn_context
Expand Down
Loading
Loading