From a78518cd9f306a2fd6987d3f686d780f204bf889 Mon Sep 17 00:00:00 2001 From: Tan Le <104194494+tanlethanh@users.noreply.github.com> Date: Thu, 27 Aug 2026 00:43:45 +0700 Subject: [PATCH] feat(host): guard codex resume against an active session Codex locks a thread for a session's whole life, so resuming one that is open in another terminal fails with "already has an active writer" and exits at once. `zedra ` runs an agent's own CLI wrapper through `AgentActor::run_wrapped`; codex is the only implementation. It probes the lock, offers kill / fork / cancel, then execs the real CLI. `resume_launch_command` routes through the wrapper, so the app hits the prompt instead of a dead terminal. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VQXVaJtBFwALs97XDrh53W --- crates/zedra-host/src/agent/codex.rs | 304 +++++++++++++++++- crates/zedra-host/src/agent/mod.rs | 47 ++- crates/zedra-host/src/global_config.rs | 3 +- crates/zedra-host/src/main.rs | 12 + docs/MANAGED_AGENTS.md | 13 + docs/MANUAL_TEST.md | 22 ++ .../src/content/docs/docs/configuration.mdx | 3 + 7 files changed, 389 insertions(+), 15 deletions(-) diff --git a/crates/zedra-host/src/agent/codex.rs b/crates/zedra-host/src/agent/codex.rs index c878be96..88d15d1f 100644 --- a/crates/zedra-host/src/agent/codex.rs +++ b/crates/zedra-host/src/agent/codex.rs @@ -3,6 +3,7 @@ use crate::sqlite_readonly; use chrono::{DateTime, Utc}; use serde::Deserialize; use serde_json::Value; +use std::io::{IsTerminal, Write}; use std::path::{Path, PathBuf}; use zedra_rpc::proto::*; @@ -697,14 +698,19 @@ impl AgentActor for CodexActor { setup_status(available, false, plugin_installed, hooks_installed, None) } - // `--no-alt-screen` runs the TUI inline so the mobile terminal keeps - // native scrollback. + // `--no-alt-screen` keeps native scrollback; the `zedra codex` prefix + // catches a session that is already open. fn resume_launch_command(&self, quoted: &str) -> Option { + let prefix = super::wrapper_prefix("codex")?; Some(format!( - "codex resume --no-alt-screen --dangerously-bypass-approvals-and-sandbox {quoted}" + "{prefix} resume --no-alt-screen --dangerously-bypass-approvals-and-sandbox {quoted}" )) } + fn run_wrapped(&self, args: &[String]) -> Option> { + Some(Self::wrap_cli(args)) + } + fn default_launch_command(&self) -> Option { self.resolved_program() .map(|_| "codex --no-alt-screen --dangerously-bypass-approvals-and-sandbox".to_string()) @@ -842,6 +848,298 @@ impl CodexActor { ]; } +enum Choice { + Kill, + Fork, + Cancel, +} + +struct Holder { + pid: u32, + detail: String, +} + +// `zedra codex ` forwards to the codex CLI, guarding `resume` against the +// per-thread writer lock (`$CODEX_HOME/thread-writer-locks/.lock`) codex +// holds for a session's whole life: resuming a session open in another terminal +// otherwise fails with "already has an active writer" and exits at once. +impl CodexActor { + fn wrap_cli(args: &[String]) -> Result<(), String> { + let Some(session_id) = Self::resume_target(args) else { + return Self::exec_codex(args.to_vec()); + }; + let Some(holder) = Self::lock_holder(&session_id) else { + return Self::exec_codex(args.to_vec()); + }; + match Self::prompt(&session_id, &holder) { + Choice::Kill => { + Self::release(&holder, &session_id)?; + Self::exec_codex(args.to_vec()) + } + Choice::Fork => Self::exec_codex(Self::fork_args(args)), + Choice::Cancel => { + println!("Cancelled."); + std::process::exit(1); + } + } + } + + /// Session id a `resume` targets; `None` when codex will pick one itself. + fn resume_target(args: &[String]) -> Option { + if args.first().map(String::as_str) != Some("resume") { + return None; + } + if let Some(id) = args.iter().skip(1).find(|arg| Self::is_session_id(arg)) { + return Some(id.clone()); + } + args.iter() + .any(|arg| arg == "--last") + .then(Self::latest_session) + .flatten() + } + + fn is_session_id(value: &str) -> bool { + value.len() == 36 + && value.bytes().enumerate().all(|(i, b)| match i { + 8 | 13 | 18 | 23 => b == b'-', + _ => b.is_ascii_hexdigit(), + }) + } + + fn latest_session() -> Option { + let cwd = std::env::current_dir().ok()?; + Self::threads_for_workdir(&cwd) + .ok()? + .into_iter() + .next() + .map(|thread| thread.id) + } + + fn lock_path(session_id: &str) -> PathBuf { + std::env::var_os("CODEX_HOME") + .map(PathBuf::from) + .unwrap_or_else(|| home_path(&[".codex"])) + .join("thread-writer-locks") + .join(format!("{session_id}.lock")) + } + + /// `None` when the session is free, or when its holder cannot be named — + /// codex then reports the conflict itself, as it did before this wrapper. + fn lock_holder(session_id: &str) -> Option { + let path = Self::lock_path(session_id); + if Self::lock_free(&path) { + return None; + } + let pid = Self::lock_pid(&path)?; + Some(Holder { + pid, + detail: Self::describe(pid), + }) + } + + /// A missing file or a probe error counts as free, so a broken probe never + /// blocks a resume codex would have allowed. + fn lock_free(path: &Path) -> bool { + let Ok(file) = std::fs::File::open(path) else { + return true; + }; + // The probe lock is released when `file` drops. + !matches!(file.try_lock(), Err(std::fs::TryLockError::WouldBlock)) + } + + fn lock_pid(path: &Path) -> Option { + let output = std::process::Command::new("lsof") + .arg("-t") + .arg(path) + .output() + .ok()?; + String::from_utf8_lossy(&output.stdout) + .split_whitespace() + .next()? + .parse() + .ok() + } + + /// `pid 123 · ttys020 · codex`; flags are dropped to fit a phone screen. + fn describe(pid: u32) -> String { + let Ok(output) = std::process::Command::new("ps") + .args(["-o", "tty=,command=", "-p", &pid.to_string()]) + .output() + else { + return format!("pid {pid}"); + }; + let line = String::from_utf8_lossy(&output.stdout); + let mut fields = line.split_whitespace(); + match (fields.next(), fields.next()) { + (Some(tty), Some(program)) => { + let program = program.rsplit('/').next().unwrap_or(program); + format!("pid {pid} \u{b7} {tty} \u{b7} {program}") + } + _ => format!("pid {pid}"), + } + } + + // One fact per line, so nothing wraps on a phone-width terminal. + fn prompt(session_id: &str, holder: &Holder) -> Choice { + let short = session_id.split('-').next().unwrap_or(session_id); + println!("\nSession {short} is open elsewhere"); + println!("{}", holder.detail); + if !std::io::stdin().is_terminal() { + println!("\nNot a terminal; cancelled."); + return Choice::Cancel; + } + println!("\n y kill it, resume here (default)"); + println!(" f fork to a new session"); + println!(" n cancel\n"); + loop { + print!("[Y/f/n] "); + let _ = std::io::stdout().flush(); + let mut line = String::new(); + if std::io::stdin().read_line(&mut line).is_err() { + return Choice::Cancel; + } + match line.trim().to_ascii_lowercase().as_str() { + "" | "y" => return Choice::Kill, + "f" => return Choice::Fork, + "n" => return Choice::Cancel, + _ => {} + } + } + } + + /// `codex fork` takes the same flags as `resume` except this one. + fn fork_args(args: &[String]) -> Vec { + args.iter() + .enumerate() + .filter(|(_, arg)| arg.as_str() != "--include-non-interactive") + .map(|(i, arg)| { + if i == 0 { + "fork".to_string() + } else { + arg.clone() + } + }) + .collect() + } + + fn release(holder: &Holder, session_id: &str) -> Result<(), String> { + let path = Self::lock_path(session_id); + Self::signal(holder.pid, false)?; + if Self::wait_free(&path, 3) { + return Ok(()); + } + Self::signal(holder.pid, true)?; + if Self::wait_free(&path, 2) { + return Ok(()); + } + Err(format!( + "codex (pid {}) still holds the session", + holder.pid + )) + } + + fn wait_free(path: &Path, secs: u64) -> bool { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(secs); + while std::time::Instant::now() < deadline { + if Self::lock_free(path) { + return true; + } + std::thread::sleep(std::time::Duration::from_millis(100)); + } + Self::lock_free(path) + } + + #[cfg(unix)] + fn signal(pid: u32, force: bool) -> Result<(), String> { + let signal = if force { libc::SIGKILL } else { libc::SIGTERM }; + // Safety: `kill` on a pid read from the lock file; failures are reported. + if unsafe { libc::kill(pid as libc::pid_t, signal) } == 0 { + return Ok(()); + } + let err = std::io::Error::last_os_error(); + match err.raw_os_error() { + Some(code) if code == libc::ESRCH => Ok(()), + _ => Err(format!("failed to signal pid {pid}: {err}")), + } + } + + #[cfg(not(unix))] + fn signal(pid: u32, _force: bool) -> Result<(), String> { + Err(format!("cannot stop pid {pid} on this platform")) + } + + #[cfg(unix)] + fn exec_codex(args: Vec) -> Result<(), String> { + use std::os::unix::process::CommandExt; + // `exec` replaces this process so the TUI owns the terminal directly. + let err = std::process::Command::new("codex").args(&args).exec(); + Err(format!("failed to run codex: {err}")) + } + + #[cfg(not(unix))] + fn exec_codex(args: Vec) -> Result<(), String> { + let status = std::process::Command::new("codex") + .args(&args) + .status() + .map_err(|err| format!("failed to run codex: {err}"))?; + std::process::exit(status.code().unwrap_or(1)); + } +} + +#[cfg(test)] +mod wrap_cli_tests { + use super::*; + + fn args(values: &[&str]) -> Vec { + values.iter().map(|v| v.to_string()).collect() + } + + #[test] + fn resume_target_reads_the_session_id_past_flags() { + let id = "01a03ed7-5678-7002-8eb1-b0d7fc14b291"; + assert_eq!( + CodexActor::resume_target(&args(&["resume", "--no-alt-screen", id])), + Some(id.to_string()) + ); + assert_eq!(CodexActor::resume_target(&args(&["fork", id])), None); + assert_eq!(CodexActor::resume_target(&args(&["resume"])), None); + assert_eq!( + CodexActor::resume_target(&args(&["resume", "-m", "gpt"])), + None + ); + } + + #[test] + fn fork_args_swap_the_verb_and_drop_the_resume_only_flag() { + let id = "01a03ed7-5678-7002-8eb1-b0d7fc14b291"; + assert_eq!( + CodexActor::fork_args(&args(&[ + "resume", + "--no-alt-screen", + "--include-non-interactive", + id, + ])), + args(&["fork", "--no-alt-screen", id]) + ); + } + + #[test] + fn session_ids_are_uuid_shaped() { + assert!(CodexActor::is_session_id( + "01a03ed7-5678-7002-8eb1-b0d7fc14b291" + )); + assert!(!CodexActor::is_session_id("--no-alt-screen")); + assert!(!CodexActor::is_session_id("01a03ed7")); + } + + #[test] + fn a_missing_lock_file_reads_as_free() { + assert!(CodexActor::lock_free(Path::new( + "/tmp/zedra-no-such-codex-lock" + ))); + } +} + #[cfg(test)] mod hook_config_tests { use super::*; diff --git a/crates/zedra-host/src/agent/mod.rs b/crates/zedra-host/src/agent/mod.rs index 357acdf1..a4450f15 100644 --- a/crates/zedra-host/src/agent/mod.rs +++ b/crates/zedra-host/src/agent/mod.rs @@ -291,6 +291,22 @@ pub fn resume_launch_command(slug: &str, session_id: &str) -> Option { .or_else(|| actor.resume_launch_command("ed)) } +/// Shell-quoted ` ` prefix that invokes an agent's wrapper. +pub fn wrapper_prefix(slug: &str) -> Option { + let exe = std::env::current_exe().ok()?; + Some(format!("{} {slug}", shell_quote(&exe.to_string_lossy()))) +} + +/// Entry point for `zedra `. +pub fn run_agent_cli(slug: &str, args: &[String]) -> Result<(), String> { + let Some(actor) = actor(slug) else { + return Err(format!("unknown command or agent: {slug}")); + }; + actor + .run_wrapped(args) + .unwrap_or_else(|| Err(format!("{slug} has no zedra CLI wrapper"))) +} + /// Agents whose sessions ignore the workdir (Hermes); cached results stay /// valid across workspace switches. pub fn is_global(slug: &str) -> bool { @@ -573,6 +589,11 @@ pub(crate) trait AgentActor: Sync { None } + /// Run `zedra `; `None` when the agent has no wrapper. + fn run_wrapped(&self, _args: &[String]) -> Option> { + None + } + /// Shell command that starts a fresh session, used when the config has no /// `launch_cmd`/`bin` override. `None` when the CLI is not on PATH. fn default_launch_command(&self) -> Option { @@ -890,8 +911,8 @@ mod tests { use super::*; #[test] - // Every resume command must launch the actor's own binary, embed the id - // shell-quoted; blank ids and unknown slugs never produce a command. + // Every resume command must launch the actor's own binary or Zedra's + // wrapper for it, with the id shell-quoted. fn resume_launch_commands_are_host_owned() { let mut resumable = 0; for actor in actors() { @@ -900,15 +921,19 @@ mod tests { continue; }; resumable += 1; - // Commands may lead with `KEY=value` env assignments before the binary. - let program = command - .split_whitespace() - .find(|token| !token.contains('=')) - .unwrap_or_default(); - assert!( - actor.programs().contains(&program), - "`{slug}` resume `{command}` does not launch one of its programs" - ); + let wrapped = wrapper_prefix(slug) + .is_some_and(|prefix| command.starts_with(&format!("{prefix} "))); + if !wrapped { + // Commands may lead with `KEY=value` env assignments before the binary. + let program = command + .split_whitespace() + .find(|token| !token.contains('=')) + .unwrap_or_default(); + assert!( + actor.programs().contains(&program), + "`{slug}` resume `{command}` does not launch one of its programs" + ); + } assert!( command.contains("ses-123"), "`{slug}` resume `{command}` drops the session id" diff --git a/crates/zedra-host/src/global_config.rs b/crates/zedra-host/src/global_config.rs index a332cf85..dc8c61c8 100644 --- a/crates/zedra-host/src/global_config.rs +++ b/crates/zedra-host/src/global_config.rs @@ -136,7 +136,8 @@ pub struct AgentConfig { pub launch_cmd: Option, /// Full resume command; `{session_id}` is replaced with the shell-quoted /// session id (e.g. `claude --dangerously-skip-permissions --resume - /// {session_id}`). Wins over the adapter's default resume. + /// {session_id}`). Wins over the adapter's default resume. Keep codex's + /// `zedra codex` prefix to keep its already-open-session prompt. pub resume_cmd: Option, } diff --git a/crates/zedra-host/src/main.rs b/crates/zedra-host/src/main.rs index 38ebcfa4..0aadf414 100644 --- a/crates/zedra-host/src/main.rs +++ b/crates/zedra-host/src/main.rs @@ -294,6 +294,11 @@ enum Commands { #[arg(value_name = "COMMAND")] command: Vec, }, + + /// Run an agent CLI through Zedra (e.g. `zedra codex resume `). + /// Every argument after the agent slug is forwarded unchanged. + #[command(external_subcommand)] + External(Vec), } #[derive(Subcommand)] @@ -1236,6 +1241,13 @@ async fn main() -> Result<()> { agent_cli::run(command).await?; } + Commands::External(args) => { + let (slug, rest) = args + .split_first() + .ok_or_else(|| anyhow::anyhow!("missing agent name"))?; + zedra_host::agent::run_agent_cli(slug, rest).map_err(|e| anyhow::anyhow!(e))?; + } + Commands::Setup { full_bin_path, no_quiet, diff --git a/docs/MANAGED_AGENTS.md b/docs/MANAGED_AGENTS.md index 8dba6b01..c00cf2fc 100644 --- a/docs/MANAGED_AGENTS.md +++ b/docs/MANAGED_AGENTS.md @@ -58,6 +58,7 @@ list. Override only what the provider supports: | Sessions & resume | `session_counts`, `sessions`, `resume_launch_command`, `scan_data_source`, `session_scan_cli` | Custom session-count types register in `session_counts_from!` (`agent/mod.rs`) | | Setup & hooks | `setup`, `setup_summary`, `supports_setup_cli`, `setup_cli`, `receive_hook`, `hook_test_payload` | `setup` is the only mutable op: writes the hook runner + provider config, returns the paths | | Account & usage | `account_fields`, `subscription_plan`, `account_usage`, `extra`, `config_files` | Async plan/usage default to `None`; skip the overrides when local-only | +| CLI wrapper | `run_wrapped` | Handles `zedra `; `None` (default) means the agent has no wrapper | | Behavior flags | `is_global`, `shows_detail` | `is_global`: sessions ignore the workdir (Hermes); `shows_detail`: listed on the app's manage screen | ### Launch commands @@ -79,6 +80,18 @@ Detect-only agents get their flags from the trailing `simple_actor!` argument; managed agents override `default_launch_command` (and put the flag in `resume_launch_command` too). +### CLI wrappers + +`zedra ` forwards to an agent's own CLI through `run_wrapped`. The +dispatcher only resolves the slug through the registry; the actor owns the rest, +because "already running" means something different for every provider. + +Codex is the one wrapper today. It locks a thread at +`$CODEX_HOME/thread-writer-locks/.lock` for the session's whole life, so a +plain `codex resume` on a session open elsewhere fails and exits. The wrapper +probes that lock, offers kill / fork / cancel, then `exec`s the real CLI. A +`resume_cmd` override keeps the guard by keeping the `zedra codex` prefix. + | Agent | Flag | Notes | | --- | --- | --- | | claude | `--dangerously-skip-permissions` | Also `CLAUDE_CODE_NO_FLICKER=0` to stay off the alt screen, so the mobile terminal keeps native scrollback | diff --git a/docs/MANUAL_TEST.md b/docs/MANUAL_TEST.md index b2d21ba1..44737bfe 100644 --- a/docs/MANUAL_TEST.md +++ b/docs/MANUAL_TEST.md @@ -2378,3 +2378,25 @@ resolution landed on 1.9.0, so the GMS error-resolution path crashed with 4. **Outdated Play Services**: on a device with an old Play Services build, repeat step 3 and confirm the "update Google Play services" resolution dialog appears instead of a crash. + +## 27. Codex Resume Guard (Session Open Elsewhere) + +Codex locks a session while it runs, so resuming one that is open elsewhere +fails and exits. `zedra codex resume` prompts instead. No model call is needed: +`codex resume` takes the lock at startup. + +1. In a desktop terminal, run `codex resume ` on a saved session and + leave it running. +2. In a second terminal, run the same id through + `cargo run -p zedra-host -- codex resume `. Expected: + `Session is open elsewhere`, `pid · ttys · codex`, then the + `y` / `f` / `n` menu and a `[Y/f/n]` prompt. +3. Answer `n`. Expected: `Cancelled.`, exit code 1, desktop codex still running. +4. Repeat, answer `f`. Expected: a new session with the same history opens under + a new id; the desktop session keeps running. +5. Repeat, press Enter. Expected: the desktop codex exits and the session + resumes here with its history. +6. `cargo run -p zedra-host -- codex --version` prints the codex version; + non-resume arguments forward unchanged. +7. From the app, tap a session open in a desktop terminal: the terminal shows + the same prompt. Tap one open nowhere: codex resumes with no prompt. diff --git a/packages/landing/src/content/docs/docs/configuration.mdx b/packages/landing/src/content/docs/docs/configuration.mdx index 92cad95d..bfa698da 100644 --- a/packages/landing/src/content/docs/docs/configuration.mdx +++ b/packages/landing/src/content/docs/docs/configuration.mdx @@ -35,6 +35,8 @@ agents: claude: bin: /opt/homebrew/bin/claude resume_cmd: claude --dangerously-skip-permissions --resume {session_id} + codex: + resume_cmd: zedra codex resume --no-alt-screen {session_id} workspace: name: My Project git: @@ -101,6 +103,7 @@ A flag or environment variable always overrides both files. Each file fills in w - **`bin`** — executable to launch instead of the one on `PATH`. - **`launch_cmd`** — full command, run verbatim. Wins over `bin`. Use it to add flags (`hermes --tui`). - **`resume_cmd`** — full command for resuming a session, wins over the built-in resume. `{session_id}` is replaced with the session id. Use it to add flags (`claude --dangerously-skip-permissions --resume {session_id}`). + - Codex locks a session while it runs, so resuming one that is open in another terminal fails. `zedra codex resume` prompts first — kill the other session, fork to a copy, or cancel — and passes every other argument through. Keep that prefix in a custom `resume_cmd` to keep the prompt. ## `workspace`