in progress cleanup

This commit is contained in:
Ryan Ragona
2025-04-26 15:45:09 -07:00
parent 07911ddc3e
commit 8ed2704191
8 changed files with 43 additions and 45 deletions

10
codex-rs/Cargo.lock generated
View File

@@ -592,6 +592,7 @@ dependencies = [
"dirs 5.0.1",
"libc",
"names",
"nanoid",
"nix 0.27.1",
"serde",
"serde_json",
@@ -2105,6 +2106,15 @@ dependencies = [
"rand 0.8.5",
]
[[package]]
name = "nanoid"
version = "0.4.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "3ffa00dec017b5b1a8b7cf5e2c008bfda1aa7e0697ac1508b491fdf2622fb4d8"
dependencies = [
"rand 0.8.5",
]
[[package]]
name = "native-tls"
version = "0.2.14"

View File

@@ -24,5 +24,3 @@ tokio = { version = "1", features = [
] }
tracing = { version = "0.1.41", features = ["log"] }
tracing-subscriber = { version = "0.3.19", features = ["env-filter"] }
# For serialising the `Cli` struct into the on-disk session metadata.

View File

@@ -2,17 +2,11 @@ use clap::ArgAction;
use clap::Parser;
use codex_core::ApprovalModeCliArg;
use codex_core::SandboxModeCliArg;
use serde::Deserialize;
use serde::Serialize;
use std::path::PathBuf;
/// Commandline arguments.
/// Command-line interface for the interactive `codex-repl` agent.
///
/// Making the struct serialisable allows us to persist the full configuration
/// inside the session metadata so we can inspect the exact flags that were
/// used to launch the session at a later time.
#[derive(Debug, Parser, Clone, Serialize, Deserialize)]
#[derive(Debug, Parser, Clone)]
#[command(
author,
version,

View File

@@ -32,9 +32,8 @@ dirs = "5"
sysinfo = "0.29"
tabwriter = "1.3"
names = { version = "0.14", default-features = false }
# unix-only process helpers
nix = { version = "0.27", default-features = false, features = ["process", "signal", "term", "fs"] }
nanoid = "0.4.0"
# Re-use the codex-exec library for its CLI definition
codex_exec = { package = "codex-exec", path = "../exec" }

View File

@@ -2,8 +2,8 @@
//!
//! The session manager can spawn two different Codex agent flavors:
//!
//! * `codex-exec` non-interactive batch agent (legacy behaviour)
//! * `codex-repl` interactive REPL that requires user input after launch
//! * `codex-exec` -- non-interactive batch agent (legacy behaviour)
//! * `codex-repl` -- interactive REPL that requires user input after launch
//!
//! The `create` command therefore has mutually exclusive sub-commands so the appropriate
//! arguments can be forwarded to the underlying agent binaries.
@@ -17,7 +17,7 @@ use chrono::SecondsFormat;
use clap::Args;
use clap::Parser;
use clap::Subcommand;
use clap::ValueEnum;
use nanoid::nanoid;
// -----------------------------------------------------------------------------
// Platform-specific imports
@@ -185,7 +185,7 @@ impl CreateCmd {
let (pid, prompt_preview, kind, argv) = match spawn_result {
Ok(tuple) => tuple,
Err(err) => {
// Best effort clean-up ignore failures so we don't mask the
// Best effort clean-up -- ignore failures so we don't mask the
// original spawn error.
let _ = store::purge(&id);
return Err(err);
@@ -208,19 +208,17 @@ impl CreateCmd {
fn truncate_preview(p: &str) -> String {
let slice: String = p.chars().take(40).collect();
if p.len() > 40 {
format!("{}", slice)
format!("{}...", slice)
} else {
slice
}
}
fn generate_session_id() -> Result<String> {
let mut generator = names::Generator::with_naming(names::Name::Numbered);
loop {
let candidate = generator.next().unwrap();
let paths = store::paths_for(&candidate)?;
if !paths.dir.exists() {
return Ok(candidate);
let id = nanoid!(8);
if !store::paths_for(&id)?.dir.exists() {
return Ok(id);
}
}
}
@@ -257,7 +255,7 @@ fn build_exec_args(cli: &codex_exec::Cli) -> Vec<String> {
fn build_repl_args(cli: &codex_repl::Cli) -> Vec<String> {
let mut args = Vec::new();
// Positional prompt argument (optional) needs to be *last* so push it later.
// Positional prompt argument (optional) -- needs to be *last* so push it later.
if let Some(model) = &cli.model {
args.push("--model".into());
@@ -273,7 +271,7 @@ fn build_repl_args(cli: &codex_repl::Cli) -> Vec<String> {
args.push("--no-ansi".into());
}
// Verbose flag is additive (-v -vv ).
// Verbose flag is additive (-v -vv ...).
for _ in 0..cli.verbose {
args.push("-v".into());
}
@@ -383,7 +381,7 @@ impl AttachCmd {
let mut reader_out = tokio::io::BufReader::new(file_out).lines();
// Conditionally open stderr if the user asked for it. Keeping the
// reader in an `Option` allows us to reuse the same select! loop the
// reader in an `Option` allows us to reuse the same select! loop -- the
// helper future simply parks forever when stderr is disabled.
let mut reader_err = if self.stderr {
let file_err = tokio::fs::File::open(&paths.stderr).await?;
@@ -406,7 +404,7 @@ impl AttachCmd {
pipe.flush().await?;
}
None => {
// Ctrl-D end of interactive input
// Ctrl-D -- end of interactive input
break;
}
}
@@ -424,7 +422,7 @@ impl AttachCmd {
// ------------------------------------------------------------------
// stderr updates (optional)
//
// To keep `tokio::select!` happy we always supply a branch when the
// To keep `tokio::select!` happy we always supply a branch -- when the
// user did *not* request stderr we hand it a future that will never
// finish (pending forever). This avoids `Option` juggling within the
// select! macro.
@@ -432,7 +430,7 @@ impl AttachCmd {
if let Some(reader) = &mut reader_err {
reader.next_line().await
} else {
// Never resolves equivalent to `futures::future::pending()`
// Never resolves -- equivalent to `futures::future::pending()`
std::future::pending().await
}
} => {
@@ -504,7 +502,7 @@ impl LogsCmd {
}
// -----------------------------------------------------------------------------
// list newest-first overview of all sessions
// list -- newest-first overview of all sessions
#[derive(Args)]
pub struct ListCmd {}

View File

@@ -1,12 +1,12 @@
//! Lightweight on-disk session metadata.
//!
//! The metadata is persisted as `meta.json` inside each session directory so
//! users or other tooling can inspect **how** a session was started even
//! users -- or other tooling -- can inspect **how** a session was started even
//! months later. Instead of serialising the full, typed CLI structs (which
//! would force every agent crate to depend on `serde`) we only keep the raw
//! argument vector that was passed to the spawned process. This keeps the
//! public API surface minimal while still giving us reproducibility a
//! session can always be re-spawned with `codex <args>`.
//! public API surface minimal while still giving us reproducibility -- a
//! session can always be re-spawned with `codex <args...>`.
use chrono::DateTime;
use chrono::Utc;
@@ -70,4 +70,3 @@ impl SessionMeta {
}
}
}

View File

@@ -40,7 +40,7 @@ fn base_command(bin: &str, paths: &Paths) -> Result<Command> {
}
// -----------------------------------------------------------------------------
// exec non-interactive batch agent
// exec -- non-interactive batch agent
pub fn spawn_exec(paths: &Paths, exec_args: &[String]) -> Result<Child> {
#[cfg(unix)]
@@ -51,7 +51,7 @@ pub fn spawn_exec(paths: &Paths, exec_args: &[String]) -> Result<Child> {
cmd.args(exec_args);
// Replace the `stdin` that `base_command` configured (null) with
// `/dev/null` opened for reading keeps the previous behaviour while
// `/dev/null` opened for reading -- keeps the previous behaviour while
// still leveraging the common helper.
let stdin = OpenOptions::new().read(true).open("/dev/null")?;
cmd.stdin(stdin);
@@ -87,7 +87,7 @@ pub fn spawn_exec(paths: &Paths, exec_args: &[String]) -> Result<Child> {
}
// -----------------------------------------------------------------------------
// repl interactive FIFO stdin
// repl -- interactive FIFO stdin
pub fn spawn_repl(paths: &Paths, repl_args: &[String]) -> Result<Child> {
#[cfg(unix)]

View File

@@ -33,11 +33,11 @@ pub struct Paths {
/// accidental creation of nested directories. Only the following ASCII
/// characters are accepted:
///
/// * `AZ`, `az`, `09`
/// * `A-Z`, `a-z`, `0-9`
/// * underscore (`_`)
/// * hyphen (`-`)
///
/// Any other byte especially path separators such as `/` or `\` results
/// Any other byte -- especially path separators such as `/` or `\\` -- results
/// in an error.
///
/// Keeping the validation local to this helper ensures that *all* call-sites
@@ -85,9 +85,9 @@ fn base_dir() -> Result<PathBuf> {
#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq)]
#[serde(rename_all = "lowercase")]
pub enum SessionKind {
/// Non-interactive batch session `codex-exec`.
/// Non-interactive batch session -- `codex-exec`.
Exec,
/// Line-oriented interactive session `codex-repl`.
/// Line-oriented interactive session -- `codex-repl`.
Repl,
}
@@ -188,7 +188,7 @@ pub fn resolve_selector(sel: &str) -> Result<String> {
/// 2. We wait for a short grace period so the process can exit cleanly.
/// 3. If the process (identified by the original PID) is still alive we force-kill it
/// with `SIGKILL` (or the Win32 `TerminateProcess` API).
/// 4. The function is **idempotent** calling it again when the session is already
/// 4. The function is **idempotent** -- calling it again when the session is already
/// terminated returns an error (`Err(AlreadyDead)`) so callers can decide whether
/// they still need to clean up the directory (`store::purge`).
///
@@ -200,7 +200,7 @@ pub async fn kill_session(id: &str) -> Result<()> {
// Resolve paths and read metadata so we know the target PID.
let paths = paths_for(id)?;
// Load meta.json we need the PID written at spawn time.
// Load meta.json -- we need the PID written at spawn time.
let bytes = std::fs::read(&paths.meta)
.with_context(|| format!("could not read metadata for session '{id}'"))?;
let meta: SessionMeta =
@@ -208,7 +208,7 @@ pub async fn kill_session(id: &str) -> Result<()> {
let pid_u32 = meta.pid;
// Helper cross-platform liveness probe based on the `sysinfo` crate.
// Helper -- cross-platform liveness probe based on the `sysinfo` crate.
fn is_alive(pid: u32) -> bool {
use sysinfo::PidExt;
use sysinfo::SystemExt;
@@ -224,12 +224,12 @@ pub async fn kill_session(id: &str) -> Result<()> {
if !still_running {
anyhow::bail!(
"session process (PID {pid_u32}) is not running directory cleanup still required"
"session process (PID {pid_u32}) is not running -- directory cleanup still required"
);
}
//---------------------------------------------------------------------
// Step 1 send graceful termination.
// Step 1 -- send graceful termination.
//---------------------------------------------------------------------
#[cfg(unix)]
@@ -265,7 +265,7 @@ pub async fn kill_session(id: &str) -> Result<()> {
}
//---------------------------------------------------------------------
// Step 2 force kill if necessary.
// Step 2 -- force kill if necessary.
//---------------------------------------------------------------------
if still_running {