chore: refactor exec() into spawn_child() and consume_truncated_output() (#878)
This PR is a straight refactor so that creating the `Child` process for an `shell` tool call and consuming its output can be separate concerns. For the actual tool call, we will always apply `consume_truncated_output()`, but for the top-level debug commands in the CLI (e.g., `debug seatbelt` and `debug landlock`), we only want to use the `spawn_child()` part of `exec()`. We want the subcommands to match the `shell` tool call usage as faithfully as possible. This becomes more important when we introduce a new parameter to `spawn_child()` in https://github.com/openai/codex/pull/879. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/878). * #879 * __->__ #878
This commit is contained in:
@@ -12,6 +12,7 @@ use std::time::Instant;
|
|||||||
use tokio::io::AsyncRead;
|
use tokio::io::AsyncRead;
|
||||||
use tokio::io::AsyncReadExt;
|
use tokio::io::AsyncReadExt;
|
||||||
use tokio::io::BufReader;
|
use tokio::io::BufReader;
|
||||||
|
use tokio::process::Child;
|
||||||
use tokio::process::Command;
|
use tokio::process::Command;
|
||||||
use tokio::sync::Notify;
|
use tokio::sync::Notify;
|
||||||
|
|
||||||
@@ -236,32 +237,42 @@ pub async fn exec(
|
|||||||
}: ExecParams,
|
}: ExecParams,
|
||||||
ctrl_c: Arc<Notify>,
|
ctrl_c: Arc<Notify>,
|
||||||
) -> Result<RawExecToolCallOutput> {
|
) -> Result<RawExecToolCallOutput> {
|
||||||
let mut child = {
|
let child = spawn_child(command, cwd).await?;
|
||||||
if command.is_empty() {
|
consume_truncated_output(child, ctrl_c, timeout_ms).await
|
||||||
return Err(CodexErr::Io(io::Error::new(
|
}
|
||||||
io::ErrorKind::InvalidInput,
|
|
||||||
"command args are empty",
|
|
||||||
)));
|
|
||||||
}
|
|
||||||
|
|
||||||
let mut cmd = Command::new(&command[0]);
|
/// Spawns the appropriate child process for the ExecParams.
|
||||||
if command.len() > 1 {
|
async fn spawn_child(command: Vec<String>, cwd: PathBuf) -> std::io::Result<Child> {
|
||||||
cmd.args(&command[1..]);
|
if command.is_empty() {
|
||||||
}
|
return Err(std::io::Error::new(
|
||||||
cmd.current_dir(cwd);
|
io::ErrorKind::InvalidInput,
|
||||||
|
"command args are empty",
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
// Do not create a file descriptor for stdin because otherwise some
|
let mut cmd = Command::new(&command[0]);
|
||||||
// commands may hang forever waiting for input. For example, ripgrep has
|
cmd.args(&command[1..]);
|
||||||
// a heuristic where it may try to read from stdin as explained here:
|
cmd.current_dir(cwd);
|
||||||
// https://github.com/BurntSushi/ripgrep/blob/e2362d4d5185d02fa857bf381e7bd52e66fafc73/crates/core/flags/hiargs.rs#L1101-L1103
|
|
||||||
cmd.stdin(Stdio::null());
|
|
||||||
|
|
||||||
cmd.stdout(Stdio::piped())
|
// Do not create a file descriptor for stdin because otherwise some
|
||||||
.stderr(Stdio::piped())
|
// commands may hang forever waiting for input. For example, ripgrep has
|
||||||
.kill_on_drop(true)
|
// a heuristic where it may try to read from stdin as explained here:
|
||||||
.spawn()?
|
// 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())
|
||||||
|
.kill_on_drop(true)
|
||||||
|
.spawn()
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Consumes the output of a child process, truncating it so it is suitable for
|
||||||
|
/// use as the output of a `shell` tool call. Also enforces specified timeout.
|
||||||
|
async fn consume_truncated_output(
|
||||||
|
mut child: Child,
|
||||||
|
ctrl_c: Arc<Notify>,
|
||||||
|
timeout_ms: Option<u64>,
|
||||||
|
) -> Result<RawExecToolCallOutput> {
|
||||||
let stdout_handle = tokio::spawn(read_capped(
|
let stdout_handle = tokio::spawn(read_capped(
|
||||||
BufReader::new(child.stdout.take().expect("stdout is not piped")),
|
BufReader::new(child.stdout.take().expect("stdout is not piped")),
|
||||||
MAX_STREAM_OUTPUT,
|
MAX_STREAM_OUTPUT,
|
||||||
|
|||||||
Reference in New Issue
Block a user