From 638253a7c7348e18fa6c00ee00e5a92de45fb760 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 11:52:46 +0000 Subject: [PATCH 01/14] fix(cli-mode): deliver large prompts via temp file + never lose the prompt on spawn failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The initial CLI prompt was single-quoted into the tmux new-session command, which tmux 3.4 rejects past ~16KB ("command too long"). The failure was invisible: create_session returned Ok when the tmux client merely spawned, and the route cleared pending_cli_prompt immediately after — so the prompt was destroyed while the pane fell back to an empty TUI. The loop supervisor's send_cli_keys hit the same ~16KB send-keys ceiling. Transport: cli_bootstrap now reads the prompt from a private 0600 file (asset_dir/cli-prompts/.txt) via `vk_p="$(cat 'file')"; ... "$vk_p"`, making the tmux command O(1) in prompt size. The prompt is never re-shell-quoted (path single-quoted; content only ever expands double-quoted). Each CliPromptArg arm keeps identical small-prompt semantics; the file self-deletes once consumed and is also removed on kill/reaper. send_cli_keys stages texts >=4KB through a namespaced tmux buffer (load-buffer - + paste-buffer -d -p) with no argv ceiling. Recovery: the route now confirms the tmux session actually exists (poll ~5x200ms) before clearing the parked prompt; if it never appears the prompt stays parked, the temp file is removed, and a red terminal error tells the user it is saved for the next attach. Oversized (>100KB) Positional/Flag prompts are delivered post-launch by paste instead of being baked past MAX_ARG_STRLEN. The loop supervisor re-parks the prompt when a wake-up delivery fails. --- crates/db/src/models/session.rs | 12 +- .../local-deployment/src/loop_supervisor.rs | 8 +- crates/local-deployment/src/pty.rs | 561 ++++++++++++++---- crates/server/src/routes/terminal.rs | 128 +++- 4 files changed, 551 insertions(+), 158 deletions(-) diff --git a/crates/db/src/models/session.rs b/crates/db/src/models/session.rs index 3496ba630f..b109dddefb 100644 --- a/crates/db/src/models/session.rs +++ b/crates/db/src/models/session.rs @@ -211,11 +211,13 @@ impl Session { } /// Read the parked CLI prompt WITHOUT clearing it. The clear is deferred - /// to [`clear_pending_cli_prompt`] after the tmux session is confirmed - /// created, so a failure between attach and spawn can't destroy the - /// user's only copy of their prompt, and two racing first-attaches that - /// both peek the same prompt are harmless — whichever wins `new-session` - /// carries it (the loser's `-A` reattach ignores its bootstrap). + /// to [`clear_pending_cli_prompt`] until the tmux session is confirmed to + /// *exist* (and, for a large paste-delivered prompt, until the paste + /// succeeds), so neither a failure between attach and spawn nor tmux + /// rejecting the launch command after spawn can destroy the user's only + /// copy of their prompt. Two racing first-attaches that both peek the same + /// prompt are harmless — whichever wins `new-session` carries it (the + /// loser's `-A` reattach ignores its bootstrap). pub async fn peek_pending_cli_prompt( pool: &SqlitePool, id: Uuid, diff --git a/crates/local-deployment/src/loop_supervisor.rs b/crates/local-deployment/src/loop_supervisor.rs index 575af2d389..b9392ddee8 100644 --- a/crates/local-deployment/src/loop_supervisor.rs +++ b/crates/local-deployment/src/loop_supervisor.rs @@ -453,7 +453,13 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), wakeup.kind.as_str() ); } else { - tracing::warn!("loop: failed to deliver wake-up to workspace {wid}"); + // Delivery failed (e.g. tmux rejected an over-long send). Re-park the + // prompt so the next terminal attach delivers it rather than dropping + // the wake-up — mirrors the session-gone branch above. + tracing::warn!("loop: failed to deliver wake-up to workspace {wid}; re-parking"); + if let Ok(Some(session)) = Session::find_latest_by_workspace_id(pool, wid).await { + let _ = Session::set_pending_cli_prompt(pool, session.id, &prompt).await; + } } ScheduledWakeup::mark_fired(pool, wakeup.id).await?; } diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 4647f84850..7c20a8610f 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -73,7 +73,7 @@ pub enum PtyCommand { fn cli_bootstrap( spec: &CliLaunchSpec, resume_session_id: Option<&str>, - initial_prompt: Option<&str>, + prompt_file: Option<&Path>, ) -> String { // The program is a bare binary name from our own code; quote it anyway so // it can never be anything but a single command word. @@ -112,27 +112,33 @@ fn cli_bootstrap( CliResume::Subcommand(sub) => format!("{prog} {sub} {id}"), CliResume::Unsupported => continue_launch(), } - } else if let Some(prompt) = initial_prompt.map(str::trim).filter(|p| !p.is_empty()) { - // CLI-first creation: the workspace prompt is delivered to the agent. + } else if let Some(file) = prompt_file { + // CLI-first creation: the workspace prompt lives in a private file + // ([`write_cli_prompt_file`]) and is read into the launch at pane-shell + // time. This keeps the tmux `new-session` command O(1) in prompt size + // (tmux rejects commands past ~16KB) and means the prompt never re-enters + // shell quoting — the file PATH is single-quoted, and the content only + // ever expands inside double quotes, so it can't be word-split or parsed + // as shell. The file self-deletes (`rm`) once consumed. + let qfile = shell_single_quote(&file.to_string_lossy()); match &spec.prompt_arg { - // Trailing positional arg (single-quote escaped — arbitrary user - // text). A leading space neutralizes prompts starting with '-' so - // they can never parse as flags. + // Trailing positional arg. The leading-dash guard and any trailing + // whitespace handling are baked into the file's contents + // ([`cli_prompt_file_content`]); command substitution strips a + // trailing newline, which is harmless. CliPromptArg::Positional => { - let guarded = if prompt.starts_with('-') { - format!(" {prompt}") - } else { - prompt.to_string() - }; - format!("{base} {}", shell_single_quote(&guarded)) + format!(r#"vk_p="$(cat {qfile})"; rm -f -- {qfile}; {base} "$vk_p""#) } - // Prompt as a flag value (e.g. gemini/copilot `-i ''`); a + // Prompt as a flag value (e.g. gemini/copilot `-i ""`); a // leading '-' is harmless after the flag. - CliPromptArg::Flag(flag) => format!("{base} {flag} {}", shell_single_quote(prompt)), + CliPromptArg::Flag(flag) => { + format!(r#"vk_p="$(cat {qfile})"; rm -f -- {qfile}; {base} {flag} "$vk_p""#) + } // Prompt piped on stdin (e.g. amp); the TUI stays interactive - // because the tmux pane keeps stdout a TTY. + // because the tmux pane keeps stdout a TTY. No argv-length ceiling + // at all. The file already carries the trailing newline printf added. CliPromptArg::StdinPipe => { - format!("printf '%s\\n' {} | {base}", shell_single_quote(prompt)) + format!("cat {qfile} | {base}; rm -f -- {qfile}") } // No CLI way to seed the prompt — start the TUI and rely on a // post-launch keystroke delivery (loop automation / send-keys). @@ -159,6 +165,98 @@ fn cli_bootstrap( ) } +/// Largest prompt (bytes) baked into the launch command via the temp-file +/// `$(cat)` transport for argv-passing agents. Positional/Flag agents hand the +/// prompt to the pane shell as a single argv entry, bounded by Linux +/// `MAX_ARG_STRLEN` (~131072 bytes); a conservative cap keeps clear of `E2BIG` +/// (and of macOS's smaller shared `ARG_MAX`). Larger prompts are delivered +/// post-launch by paste instead (see [`cli_prompt_fits_inline`]). +const MAX_INLINE_PROMPT_BYTES: usize = 100_000; + +/// Whether an initial prompt of `byte_len` bytes can be baked into the launch +/// command for an agent with this `prompt_arg`, or must be delivered after the +/// TUI is up (via [`send_cli_keys`]). `StdinPipe` has no argv ceiling; +/// `Positional`/`Flag` pass the prompt as one argv entry and are capped; +/// `Unsupported` has no launch-time transport at all. +pub fn cli_prompt_fits_inline(prompt_arg: &CliPromptArg, byte_len: usize) -> bool { + match prompt_arg { + CliPromptArg::Positional | CliPromptArg::Flag(_) => byte_len <= MAX_INLINE_PROMPT_BYTES, + CliPromptArg::StdinPipe => true, + CliPromptArg::Unsupported => false, + } +} + +/// The exact bytes to write to a workspace's CLI prompt file for `prompt_arg`, +/// or `None` when the (trimmed) prompt is blank — mirroring the old in-command +/// quoting semantics so small prompts behave identically: the leading-dash +/// guard for `Positional` (so a prompt like `-rf` can't parse as a flag) is a +/// literal leading space in the file; `StdinPipe` keeps the trailing newline the +/// old `printf '%s\n'` added. The content is stored verbatim (never +/// shell-escaped) — the bootstrap reads it back inside double quotes. +fn cli_prompt_file_content(prompt_arg: &CliPromptArg, prompt: &str) -> Option { + let prompt = prompt.trim(); + if prompt.is_empty() { + return None; + } + Some(match prompt_arg { + CliPromptArg::Positional => { + if prompt.starts_with('-') { + format!(" {prompt}") + } else { + prompt.to_string() + } + } + CliPromptArg::Flag(_) => prompt.to_string(), + CliPromptArg::StdinPipe => format!("{prompt}\n"), + CliPromptArg::Unsupported => return None, + }) +} + +/// Path of a workspace's transient CLI initial-prompt file. Kept next to the +/// other backend assets (same trust domain as the SQLite DB) under a dedicated +/// `cli-prompts/` subdir; named by the workspace id so racing first-attaches +/// write the same path idempotently. +fn cli_prompt_file_path(workspace_id: Uuid) -> PathBuf { + utils::assets::asset_dir() + .join("cli-prompts") + .join(format!("{}.txt", workspace_id.simple())) +} + +/// Write a workspace's CLI initial prompt to its private (0600) file for the +/// bootstrap to read. Returns the path on success. The file self-deletes once +/// the bootstrap consumes it; [`remove_cli_prompt_file`] and +/// [`kill_cli_tmux_session`] clean up the never-consumed case. +fn write_cli_prompt_file(workspace_id: Uuid, content: &str) -> std::io::Result { + let path = cli_prompt_file_path(workspace_id); + if let Some(dir) = path.parent() { + std::fs::create_dir_all(dir)?; + } + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + let mut f = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(&path)?; + f.write_all(content.as_bytes())?; + } + #[cfg(not(unix))] + { + std::fs::write(&path, content.as_bytes())?; + } + Ok(path) +} + +/// Best-effort delete of a workspace's transient CLI prompt file. Called when a +/// session that never consumed it is torn down (spawn failure recovery, kill, +/// reaper) so a prompt is never left readable on disk after it's no longer +/// needed. +pub fn remove_cli_prompt_file(workspace_id: Uuid) { + let _ = std::fs::remove_file(cli_prompt_file_path(workspace_id)); +} + /// How to install each interactive-CLI agent, shown in the pane when its binary /// isn't on PATH. Kept here (next to the bootstrap that prints it) rather than on /// the spec so the message stays a deployment concern. @@ -680,6 +778,9 @@ pub(crate) fn workspace_id_from_cli_session_name(name: &str) -> Option { /// cleanup so sessions don't outlive their worktree). `=` forces exact-name /// matching — tmux `-t` is otherwise a prefix match. pub async fn kill_cli_tmux_session(workspace_id: Uuid) { + // Drop any transient prompt file alongside the session (covers the + // never-attached case where the bootstrap never ran to self-delete it). + remove_cli_prompt_file(workspace_id); if !tmux_available() { return; } @@ -736,22 +837,43 @@ pub async fn capture_cli_pane(workspace_id: Uuid) -> Option { Some(String::from_utf8_lossy(&output.stdout).into_owned()) } +/// Texts at/above this size go through tmux buffers (load-buffer + paste-buffer) +/// instead of `send-keys -l`, which — like every tmux client command — is +/// rejected once its argv exceeds ~16KB. The small-text `send-keys` path is the +/// already-live-verified one, so keep it for the common case. +const SEND_KEYS_PASTE_THRESHOLD: usize = 4096; + +/// Run a fire-and-forget tmux command on our socket, reporting only success. +async fn tmux_ok(args: &[&str]) -> bool { + tokio::process::Command::new("tmux") + .args(args) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .await + .map(|s| s.success()) + .unwrap_or(false) +} + /// Type `text` into a workspace's CLI tmux pane and submit it (Enter), as if the /// user typed it. This is the only way to re-prompt a LIVE, detached agent: the /// parked `pending_cli_prompt` path only fires when a fresh tmux session is -/// created, so an already-running pane needs keystroke injection. `-l` sends the -/// text literally so it can never be interpreted as tmux key names; Enter is a +/// created, so an already-running pane needs keystroke injection. Small texts go +/// via `send-keys -l` (literal, so never interpreted as tmux key names); larger +/// texts are staged through a namespaced tmux buffer and bracketed-pasted (no +/// argv-length ceiling; embedded newlines don't submit early). Enter is a /// separate call so it submits rather than being typed verbatim. Best-effort. pub async fn send_cli_keys(workspace_id: Uuid, text: &str) -> bool { if !tmux_available() { return false; } - // Bare name (not `=exact`): send-keys takes a pane target, for which the - // `=` session-target syntax is rejected. Unambiguous given full-hex names. + // Bare name (not `=exact`): send-keys/paste-buffer take a pane target, for + // which the `=` session-target syntax is rejected. Unambiguous given + // full-hex names. let target = cli_tmux_session_name(workspace_id); - let typed = tokio::process::Command::new("tmux") - .args([ + let delivered = if text.len() < SEND_KEYS_PASTE_THRESHOLD { + tmux_ok(&[ "-L", CLI_TMUX_SOCKET, "send-keys", @@ -760,24 +882,65 @@ pub async fn send_cli_keys(workspace_id: Uuid, text: &str) -> bool { "-l", text, ]) - .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()) - .status() .await - .map(|s| s.success()) - .unwrap_or(false); - if !typed { + } else { + paste_via_tmux_buffer(workspace_id, &target, text).await + }; + if !delivered { return false; } - tokio::process::Command::new("tmux") - .args(["-L", CLI_TMUX_SOCKET, "send-keys", "-t", &target, "Enter"]) + tmux_ok(&["-L", CLI_TMUX_SOCKET, "send-keys", "-t", &target, "Enter"]).await +} + +/// Stage `text` into a per-workspace tmux buffer via `load-buffer -` (text on +/// stdin, so no argv-length limit) and bracketed-paste it into the pane. The +/// buffer is namespaced (`vk_prompt_`) and deleted on paste (`-d`) because +/// tmux buffers are server-global — otherwise concurrent workspaces could +/// cross-deliver. `-p` (bracketed paste) makes the TUI treat multi-line text as +/// one paste so embedded newlines don't submit early. +async fn paste_via_tmux_buffer(workspace_id: Uuid, target: &str, text: &str) -> bool { + use tokio::io::AsyncWriteExt; + + let buffer = format!("vk_prompt_{}", workspace_id.simple()); + + let mut child = match tokio::process::Command::new("tmux") + .args(["-L", CLI_TMUX_SOCKET, "load-buffer", "-b", &buffer, "-"]) + .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) - .status() - .await - .map(|s| s.success()) - .unwrap_or(false) + .spawn() + { + Ok(child) => child, + Err(_) => return false, + }; + + if let Some(mut stdin) = child.stdin.take() { + if stdin.write_all(text.as_bytes()).await.is_err() { + return false; + } + // Drop stdin (EOF) so load-buffer completes. + let _ = stdin.shutdown().await; + drop(stdin); + } + + let loaded = child.wait().await.map(|s| s.success()).unwrap_or(false); + if !loaded { + return false; + } + + tmux_ok(&[ + "-L", + CLI_TMUX_SOCKET, + "paste-buffer", + "-d", + "-p", + "-b", + &buffer, + "-t", + target, + ]) + .await } /// Seconds since the Unix epoch (best-effort; 0 if the system clock is before @@ -982,86 +1145,112 @@ impl PtyService { } } - let (mut cmd, shell_name) = - if let (Some(session_name), Some(spec)) = (&tmux_session, &tmux_spec) { - // Bring an already-running server in line with our config - // (options are server-wide; `-f` below only affects a fresh - // server start). - ensure_cli_tmux_server_options(); - - // Pre-accept the agent's per-directory folder-trust / first-run - // dialog for this app-created worktree so the launch never - // blocks on it. - maybe_seed_cli_trust(&spec.program, &working_dir); - - let mut cmd = CommandBuilder::new("tmux"); - // Our own config instead of the user's ~/.tmux.conf — the - // embedded terminal needs deterministic mouse/clipboard - // behavior (see CLI_TMUX_CONF); the user's personal tmux on - // the default socket is unaffected. - if let Some(conf) = cli_tmux_conf_path() { - cmd.arg("-f"); - cmd.arg(conf); - } - // Dedicated socket isolates our sessions from the user's tmux. - cmd.arg("-L"); - cmd.arg(CLI_TMUX_SOCKET); - cmd.arg("new-session"); - // -A: attach if the session exists, else create. - // - // We deliberately do NOT pass -D (detach other clients): a new - // attach would detach the prior client, whose tmux process then - // exits → its PTY hits EOF → the WebSocket closes → the frontend - // reconnects → the new attach detaches it again, a self- - // sustaining reconnect loop that also resets the session to the - // attaching client's 80x24 default on every cycle. Without -D, - // reconnects simply attach; the prior client is cleaned up by - // close_session killing its PTY child. Two simultaneous browser - // windows would mirror (tmux sizes to the smaller) — a rare, - // benign trade vs. the loop. - cmd.arg("-A"); - cmd.arg("-s"); - cmd.arg(session_name); - cmd.arg("-c"); - cmd.arg(&working_dir); - cmd.arg(cli_bootstrap( - spec, - tmux_resume_id.as_deref(), - tmux_initial_prompt.as_deref(), - )); - cmd.cwd(&working_dir); - // No shell-specific prompt configuration for the tmux client. - (cmd, String::new()) + let (mut cmd, shell_name) = if let (Some(session_name), Some(spec)) = + (&tmux_session, &tmux_spec) + { + // Bring an already-running server in line with our config + // (options are server-wide; `-f` below only affects a fresh + // server start). + ensure_cli_tmux_server_options(); + + // Pre-accept the agent's per-directory folder-trust / first-run + // dialog for this app-created worktree so the launch never + // blocks on it. + maybe_seed_cli_trust(&spec.program, &working_dir); + + let mut cmd = CommandBuilder::new("tmux"); + // Our own config instead of the user's ~/.tmux.conf — the + // embedded terminal needs deterministic mouse/clipboard + // behavior (see CLI_TMUX_CONF); the user's personal tmux on + // the default socket is unaffected. + if let Some(conf) = cli_tmux_conf_path() { + cmd.arg("-f"); + cmd.arg(conf); + } + // Dedicated socket isolates our sessions from the user's tmux. + cmd.arg("-L"); + cmd.arg(CLI_TMUX_SOCKET); + cmd.arg("new-session"); + // -A: attach if the session exists, else create. + // + // We deliberately do NOT pass -D (detach other clients): a new + // attach would detach the prior client, whose tmux process then + // exits → its PTY hits EOF → the WebSocket closes → the frontend + // reconnects → the new attach detaches it again, a self- + // sustaining reconnect loop that also resets the session to the + // attaching client's 80x24 default on every cycle. Without -D, + // reconnects simply attach; the prior client is cleaned up by + // close_session killing its PTY child. Two simultaneous browser + // windows would mirror (tmux sizes to the smaller) — a rare, + // benign trade vs. the loop. + cmd.arg("-A"); + cmd.arg("-s"); + cmd.arg(session_name); + cmd.arg("-c"); + cmd.arg(&working_dir); + // Materialize the initial prompt to a private file so the + // bootstrap reads it back rather than carrying it inline + // (tmux rejects `new-session` commands past ~16KB). Only when + // an existing conversation won't take precedence and the + // prompt isn't blank; the file self-deletes once consumed. + let resume_active = tmux_resume_id.as_deref().is_some_and(is_uuid); + let prompt_file: Option = if resume_active { + None + } else { + tmux_initial_prompt + .as_deref() + .and_then(|p| cli_prompt_file_content(&spec.prompt_arg, p)) + .and_then(|content| { + let wid = workspace_id_from_cli_session_name(session_name)?; + match write_cli_prompt_file(wid, &content) { + Ok(path) => Some(path), + Err(e) => { + tracing::warn!( + "Failed to write CLI prompt file for {session_name}: {e}" + ); + None + } + } + }) + }; + cmd.arg(cli_bootstrap( + spec, + tmux_resume_id.as_deref(), + prompt_file.as_deref(), + )); + cmd.cwd(&working_dir); + // No shell-specific prompt configuration for the tmux client. + (cmd, String::new()) + } else { + let mut cmd = CommandBuilder::new(&shell); + cmd.cwd(&working_dir); + + // Configure shell-specific options + let shell_name = shell + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or("") + .to_string(); + + if shell_name == "powershell.exe" || shell_name == "pwsh.exe" { + // PowerShell: use -NoLogo for cleaner startup + cmd.arg("-NoLogo"); + } else if shell_name == "cmd.exe" { + // cmd.exe: no special args needed } else { - let mut cmd = CommandBuilder::new(&shell); - cmd.cwd(&working_dir); - - // Configure shell-specific options - let shell_name = shell - .file_name() - .and_then(|n| n.to_str()) - .unwrap_or("") - .to_string(); - - if shell_name == "powershell.exe" || shell_name == "pwsh.exe" { - // PowerShell: use -NoLogo for cleaner startup - cmd.arg("-NoLogo"); - } else if shell_name == "cmd.exe" { - // cmd.exe: no special args needed + // Unix shells + cmd.env("VIBE_KANBAN_TERMINAL", "1"); + + if shell_name == "bash" { + cmd.env("PROMPT_COMMAND", r#"PS1='$ '; unset PROMPT_COMMAND"#); + } else if shell_name == "zsh" { + // PROMPT is set after spawning } else { - // Unix shells - cmd.env("VIBE_KANBAN_TERMINAL", "1"); - - if shell_name == "bash" { - cmd.env("PROMPT_COMMAND", r#"PS1='$ '; unset PROMPT_COMMAND"#); - } else if shell_name == "zsh" { - // PROMPT is set after spawning - } else { - cmd.env("PS1", "$ "); - } + cmd.env("PS1", "$ "); } - (cmd, shell_name) - }; + } + (cmd, shell_name) + }; cmd.env("TERM", "xterm-256color"); cmd.env("COLORTERM", "truecolor"); @@ -1306,9 +1495,15 @@ mod tests { // A valid session UUID -> --resume , even if a prompt is also // present (an existing conversation always wins). let id = "28b98f08-5f5f-4b1e-8c4e-41ae87c0c706"; - let b = cli_bootstrap(&claude_spec(&[]), Some(id), Some("do things")); + let b = cli_bootstrap( + &claude_spec(&[]), + Some(id), + Some(Path::new("/tmp/vk/prompt.txt")), + ); assert!(b.contains(&format!("--resume {id}"))); - assert!(!b.contains("do things")); + // The prompt file is ignored entirely when resuming. + assert!(!b.contains("prompt.txt")); + assert!(!b.contains("vk_p=")); // Non-UUID (injection attempt) is rejected and never interpolated. let evil = "x; rm -rf ~"; let b = cli_bootstrap(&claude_spec(&[]), Some(evil), None); @@ -1334,27 +1529,131 @@ mod tests { } #[test] - fn cli_bootstrap_passes_initial_prompt_injection_safe() { + fn cli_bootstrap_reads_prompt_from_file_length_is_constant() { + // The prompt is delivered via a temp file, so the generated command is + // O(1) in prompt size — the whole point of the fix (tmux rejects + // commands past ~16KB). The file PATH is single-quoted; the content is + // only ever expanded inside double quotes, so it can never be + // word-split or parsed as shell (injection-safe by construction). let spec = claude_spec(&["--dangerously-skip-permissions"]); - let b = cli_bootstrap(&spec, None, Some("Fix the login bug")); - assert!(b.contains("'--dangerously-skip-permissions' 'Fix the login bug'")); + let file = Path::new("/tmp/vk/cli-prompts/abc.txt"); + let b = cli_bootstrap(&spec, None, Some(file)); + assert!( + b.len() < 2048, + "bootstrap must stay small regardless of prompt size: {} bytes", + b.len() + ); + // Path single-quoted, expansion double-quoted, file self-deletes. + assert!(b.contains("vk_p=\"$(cat '/tmp/vk/cli-prompts/abc.txt')\"")); + assert!(b.contains("rm -f -- '/tmp/vk/cli-prompts/abc.txt'")); + assert!( + b.contains("'--dangerously-skip-permissions' \"$vk_p\""), + "positional prompt expands double-quoted after the flags: {b}" + ); - // Quotes and shell metacharacters stay inert inside the quoting. - let evil = "'; rm -rf ~; echo '"; + // A prompt file whose path contains a quote can't break out of the + // single-quoting (defense in depth; real paths are workspace hex): the + // embedded quote is POSIX-escaped as `'\''`, so the dangerous run stays + // inert data inside the quoting rather than terminating it. + let evil = Path::new("/tmp/'; rm -rf ~; echo '.txt"); let b = cli_bootstrap(&spec, None, Some(evil)); - // The single quotes in the prompt are escaped as '\'' — the raw - // sequence `'; rm` can therefore never terminate the quoting. - assert!(b.contains(r"'"), "quotes must be escaped: {b}"); - assert!(!b.contains("&& rm"), "injection must not escape: {b}"); - - // A prompt starting with '-' is space-guarded so the agent can't parse - // it as a flag. - let dashy = cli_bootstrap(&spec, None, Some("-rf is a flag-looking prompt")); - assert!(dashy.contains("' -rf is a flag-looking prompt'")); - - // Blank prompts fall through to the no-prompt path. - let blank = cli_bootstrap(&spec, None, Some(" ")); - assert!(blank.contains("--continue || 'claude'")); + assert!( + b.contains(r"'\''; rm -rf ~; echo '\''"), + "path quote must be escaped, not terminated: {b}" + ); + // The raw, unescaped break-out (a bare `'` closing the cat quote right + // before the command) must never appear. + assert!( + !b.contains("cat '/tmp/'; rm"), + "quoting must not break out: {b}" + ); + } + + #[test] + fn cli_bootstrap_flag_and_stdin_prompt_forms_read_from_file() { + let file = Path::new("/tmp/vk/p.txt"); + + // Flag agents expand the file into the flag's value, double-quoted. + let flag_spec = CliLaunchSpec::new("gemini", vec![]) + .with_prompt_arg(CliPromptArg::Flag("-i".to_string())); + let b = cli_bootstrap(&flag_spec, None, Some(file)); + assert!(b.contains("rm -f -- '/tmp/vk/p.txt'; 'gemini' -i \"$vk_p\"")); + + // StdinPipe agents pipe the file into the program — no argv ceiling. + let pipe_spec = CliLaunchSpec::new("amp", vec![]).with_prompt_arg(CliPromptArg::StdinPipe); + let b = cli_bootstrap(&pipe_spec, None, Some(file)); + assert!(b.contains("cat '/tmp/vk/p.txt' | 'amp'; rm -f -- '/tmp/vk/p.txt'")); + } + + #[test] + fn cli_bootstrap_no_prompt_file_falls_through_to_continue() { + // No prompt file (blank prompt filtered out by the caller) -> the + // no-prompt continue/fresh path, exactly as before. + let spec = claude_spec(&["--dangerously-skip-permissions"]); + let b = cli_bootstrap(&spec, None, None); + assert!(b.contains("--continue || 'claude'")); + assert!(!b.contains("vk_p=")); + } + + #[test] + fn cli_prompt_file_content_matches_old_quoting_semantics() { + // Blank (after trim) -> no file is written (falls through to continue). + assert_eq!( + cli_prompt_file_content(&CliPromptArg::Positional, " "), + None + ); + + // Positional: stored verbatim, byte-exact — quotes/metacharacters are + // NOT escaped (the bootstrap reads it back inside double quotes). + let evil = "'; rm -rf ~; echo '"; + assert_eq!( + cli_prompt_file_content(&CliPromptArg::Positional, evil).as_deref(), + Some(evil) + ); + + // Positional leading-dash guard becomes a literal leading space in the + // file so the agent can't parse the prompt as a flag. + assert_eq!( + cli_prompt_file_content(&CliPromptArg::Positional, "-rf is a prompt").as_deref(), + Some(" -rf is a prompt") + ); + + // Flag: no dash guard needed (the value follows a flag). + assert_eq!( + cli_prompt_file_content(&CliPromptArg::Flag("-i".to_string()), "-x").as_deref(), + Some("-x") + ); + + // StdinPipe keeps the trailing newline the old `printf '%s\n'` added. + assert_eq!( + cli_prompt_file_content(&CliPromptArg::StdinPipe, "hello").as_deref(), + Some("hello\n") + ); + + // Unsupported agents have no launch-time transport. + assert_eq!( + cli_prompt_file_content(&CliPromptArg::Unsupported, "hi"), + None + ); + } + + #[test] + fn cli_prompt_fits_inline_caps_argv_agents_only() { + // Positional/Flag are capped (single argv entry, Linux MAX_ARG_STRLEN). + assert!(cli_prompt_fits_inline(&CliPromptArg::Positional, 100_000)); + assert!(!cli_prompt_fits_inline(&CliPromptArg::Positional, 100_001)); + assert!(cli_prompt_fits_inline( + &CliPromptArg::Flag("-i".to_string()), + 100_000 + )); + assert!(!cli_prompt_fits_inline( + &CliPromptArg::Flag("-i".to_string()), + 200_000 + )); + // StdinPipe has no argv ceiling. + assert!(cli_prompt_fits_inline(&CliPromptArg::StdinPipe, 5_000_000)); + // Unsupported never bakes in. + assert!(!cli_prompt_fits_inline(&CliPromptArg::Unsupported, 1)); } #[test] diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 750eea83ef..c73dd44e89 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -21,7 +21,8 @@ use executors::{ profile::{ExecutorConfig, ExecutorConfigs}, }; use local_deployment::pty::{ - PtyCommand, cli_tmux_available, cli_tmux_session_exists, cli_tmux_session_name, + PtyCommand, cli_prompt_fits_inline, cli_tmux_available, cli_tmux_session_exists, + cli_tmux_session_name, remove_cli_prompt_file, send_cli_keys, }; use serde::{Deserialize, Serialize}; use sqlx::SqlitePool; @@ -187,6 +188,10 @@ async fn terminal_ws( // is confirmed created (CLI-first first attach only). Set inside the Cli // arm; cleared post-spawn in handle_terminal_ws. let mut prompt_session_to_clear: Option = None; + // A large (or otherwise non-inlineable) initial prompt that must be + // delivered by paste AFTER the pane is up, rather than baked into the launch + // command. Set inside the Cli arm; delivered in handle_terminal_ws. + let mut deferred_prompt: Option = None; let (working_dir, command) = match query.mode { TerminalMode::Cli => { @@ -297,23 +302,43 @@ async fn terminal_ws( } _ => None, }; - // Remember which session's prompt to clear once the PTY/tmux - // session is up (only when we actually carried a prompt). - if initial_prompt.is_some() { - prompt_session_to_clear = session.as_ref().map(|s| s.id); - } - // Honor the workspace's selected agent + model/effort at launch // (defaults to claude at Opus/max when nothing was selected). let (model_id, reasoning_id) = resolve_cli_model_effort(pool, session.as_ref()).await; let spec = resolve_cli_launch_spec(session.as_ref(), model_id, reasoning_id, &dir); + // Small prompts ride the bootstrap's temp-file transport (baked into + // the launch). Prompts too large to pass as one argv entry — or + // agents with no launch-time prompt arg — are delivered by paste + // after the pane is confirmed up (see handle_terminal_ws), so they're + // never truncated and never silently lost. Either way the parked + // prompt is cleared only after delivery is confirmed. + let baked_prompt = match initial_prompt { + Some(prompt) => { + let trimmed = prompt.trim(); + if trimmed.is_empty() { + None + } else if cli_prompt_fits_inline(&spec.prompt_arg, trimmed.len()) { + Some(prompt) + } else { + deferred_prompt = Some(trimmed.to_string()); + None + } + } + None => None, + }; + // Remember which session's prompt to clear once delivery is + // confirmed (only when we actually carried a prompt). + if baked_prompt.is_some() || deferred_prompt.is_some() { + prompt_session_to_clear = session.as_ref().map(|s| s.id); + } + ( dir, PtyCommand::TmuxCli { session_name: cli_tmux_session_name(query.workspace_id), resume_session_id, - initial_prompt, + initial_prompt: baked_prompt, spec, }, ) @@ -352,11 +377,14 @@ async fn terminal_ws( query.cols, query.rows, command, + query.workspace_id, prompt_session_to_clear, + deferred_prompt, ) })) } +#[allow(clippy::too_many_arguments)] async fn handle_terminal_ws( mut socket: MaybeSignedWebSocket, deployment: DeploymentImpl, @@ -364,7 +392,9 @@ async fn handle_terminal_ws( cols: u16, rows: u16, command: PtyCommand, + workspace_id: Uuid, prompt_session_to_clear: Option, + deferred_prompt: Option, ) { let (session_id, mut output_rx) = match deployment .pty() @@ -379,19 +409,58 @@ async fn handle_terminal_ws( } }; - // The tmux session is now created and its bootstrap (carrying the parked - // CLI prompt) is running; only now is it safe to clear the prompt, so a - // failure before this point leaves it parked for the next attach. If the - // clear itself fails the prompt stays parked and a later attach (after a - // tmux death) could replay it — narrow, but log so it's observable. - if let Some(session_id) = prompt_session_to_clear - && let Err(e) = Session::clear_pending_cli_prompt(&deployment.db().pool, session_id).await - { - tracing::warn!( - "Failed to clear delivered CLI prompt for session {}: {}", - session_id, - e - ); + // create_session returning Ok only means the tmux *client* process spawned; + // the tmux server can still reject the command moments later (historically: + // an over-long prompt baked into `new-session`). So confirm the session + // actually exists before clearing the parked prompt — and, for a deferred + // large prompt, before pasting it in. A session that never comes up leaves + // the prompt saved for the next attach and tells the user so, instead of + // silently destroying it. + if let Some(clear_session_id) = prompt_session_to_clear { + if wait_for_cli_session(workspace_id).await { + let delivered = match &deferred_prompt { + Some(text) => { + // Give the freshly-launched TUI a moment to be ready to + // accept a paste, then deliver the oversized prompt via the + // buffer path (no argv-length ceiling). + tokio::time::sleep(std::time::Duration::from_millis(750)).await; + send_cli_keys(workspace_id, text).await + } + None => true, + }; + if delivered { + if let Err(e) = + Session::clear_pending_cli_prompt(&deployment.db().pool, clear_session_id).await + { + tracing::warn!( + "Failed to clear delivered CLI prompt for session {}: {}", + clear_session_id, + e + ); + } + } else { + // Leave the prompt parked; the next attach retries delivery. + tracing::warn!( + "Failed to deliver large CLI prompt to workspace {}; left parked", + workspace_id + ); + } + } else { + // The session never appeared — remove the transient prompt file (the + // DB copy stays parked) and surface the failure. The frontend renders + // this in red and halts its reconnect loop. + remove_cli_prompt_file(workspace_id); + tracing::error!( + "CLI tmux session for workspace {} never came up; prompt left parked", + workspace_id + ); + let _ = send_error( + &mut socket, + "Failed to start the agent session — your prompt is saved and will be delivered on the next attach", + ) + .await; + return; + } } let pty_service = deployment.pty().clone(); @@ -447,6 +516,23 @@ async fn handle_terminal_ws( let _ = deployment.pty().close_session(session_id).await; } +/// Poll for the workspace's CLI tmux session to appear after a spawn. The +/// session existing proves tmux accepted `new-session` and the bootstrap (which +/// owns the prompt file) is running; only then is it safe to clear the parked +/// prompt. Short backoff (~5 × 200ms) — the session shows up immediately when +/// tmux accepts the command, so this only spins when it's failing. +async fn wait_for_cli_session(workspace_id: Uuid) -> bool { + for attempt in 0..5 { + if cli_tmux_session_exists(workspace_id).await { + return true; + } + if attempt < 4 { + tokio::time::sleep(std::time::Duration::from_millis(200)).await; + } + } + false +} + async fn send_error(socket: &mut MaybeSignedWebSocket, message: &str) -> anyhow::Result<()> { let msg = TerminalMessage::Error { message: message.to_string(), From fca4a5c4626765cdde7c51561cd64df2eed43036 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 12:12:05 +0000 Subject: [PATCH 02/14] fix(cli-mode): never drop a parked prompt when staging its temp file fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If write_cli_prompt_file() failed while baking a small/mid initial prompt (full or read-only FS, or a permissions problem creating cli-prompts/), the bootstrap silently fell through to continue_launch() — an empty TUI on a fresh worktree — and, because the tmux session came up healthy, terminal.rs cleared the parked prompt: the user's only copy was destroyed, gated on an I/O error rather than tmux's old 16KB limit. This violated the 'never destroy the prompt' invariant. create_session now fails the spawn with a dedicated PtyError::PromptStageFailed when there is prompt content to deliver but its file cannot be staged, instead of dropping it. terminal.rs's create_session Err path then returns before the prompt-clear, leaving the DB copy parked for the next attach and surfacing the same red recovery notice as the 'session never came up' branch (now a shared CLI_PROMPT_PARKED_NOTICE constant so the two paths can't drift). Any partial prompt file is removed so a torn write can't leave a stale prompt on disk. --- crates/local-deployment/src/pty.rs | 74 +++++++++++++++++++++++----- crates/server/src/routes/terminal.rs | 10 ++-- 2 files changed, 65 insertions(+), 19 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 7c20a8610f..40f71c97fa 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -1040,10 +1040,23 @@ pub(crate) fn tmux_available() -> bool { const TMUX_MISSING_NOTICE: &[u8] = b"\x1b[33m\xe2\x9a\xa0 tmux not found \xe2\x80\x94 running an ephemeral shell; this session will NOT survive disconnects.\x1b[0m\r\n"; +/// User-facing copy shown (in red, halting the reconnect loop) whenever a CLI +/// attach fails in a way that leaves the parked prompt intact for the next +/// attach. Shared so the "prompt staging failed" and "session never came up" +/// recovery paths speak with one voice. +pub const CLI_PROMPT_PARKED_NOTICE: &str = "Failed to start the agent session — your prompt is saved and will be delivered on the next attach"; + #[derive(Debug, Error)] pub enum PtyError { #[error("Failed to create PTY: {0}")] CreateFailed(String), + /// A parked initial prompt could not be staged for delivery (e.g. the + /// prompt file write failed on a full/read-only FS). Distinct from + /// `CreateFailed` so the message reaches the user verbatim — and so we + /// never silently drop the prompt by falling through to an empty + /// `continue_launch` TUI. The parked DB copy is left untouched for retry. + #[error("{0}")] + PromptStageFailed(String), #[error("Session not found: {0}")] SessionNotFound(Uuid), #[error("Failed to write to PTY: {0}")] @@ -1197,21 +1210,44 @@ impl PtyService { let prompt_file: Option = if resume_active { None } else { - tmux_initial_prompt + match tmux_initial_prompt .as_deref() .and_then(|p| cli_prompt_file_content(&spec.prompt_arg, p)) - .and_then(|content| { - let wid = workspace_id_from_cli_session_name(session_name)?; - match write_cli_prompt_file(wid, &content) { - Ok(path) => Some(path), - Err(e) => { - tracing::warn!( - "Failed to write CLI prompt file for {session_name}: {e}" + { + // There is a prompt to deliver: its file MUST be staged. + // If the write fails we CANNOT fall through to + // `continue_launch` — that yields a healthy-looking but + // empty TUI, and the caller (terminal.rs) would then + // clear the parked DB copy, permanently losing the user's + // only copy of the prompt. Fail the spawn instead so the + // parked prompt survives for the next attach and the user + // sees the recovery notice (the "never destroy the + // prompt" invariant). + Some(content) => { + let wid = workspace_id_from_cli_session_name(session_name).ok_or_else( + || { + tracing::error!( + "CLI session name {session_name} has no workspace id; \ + cannot stage parked prompt" ); - None - } - } - }) + PtyError::PromptStageFailed( + CLI_PROMPT_PARKED_NOTICE.to_string(), + ) + }, + )?; + Some(write_cli_prompt_file(wid, &content).map_err(|e| { + tracing::error!( + "Failed to write CLI prompt file for {session_name}: \ + {e}; leaving prompt parked" + ); + // Drop any partial file so a torn write can't + // leave a stale prompt readable on disk. + remove_cli_prompt_file(wid); + PtyError::PromptStageFailed(CLI_PROMPT_PARKED_NOTICE.to_string()) + })?) + } + None => None, + } }; cmd.arg(cli_bootstrap( spec, @@ -1637,6 +1673,20 @@ mod tests { ); } + #[test] + fn prompt_stage_failed_surfaces_recovery_notice_verbatim() { + // When staging a parked prompt fails (e.g. a full/read-only FS), the + // spawn is aborted with PromptStageFailed rather than silently dropping + // the prompt. The error text must reach the user verbatim (terminal.rs + // sends `e.to_string()` to the pane), so it carries exactly the same + // recovery copy as the "session never came up" branch — telling the user + // the prompt is saved for the next attach. Guards the user-facing half + // of the "never destroy the prompt" invariant. + let err = PtyError::PromptStageFailed(CLI_PROMPT_PARKED_NOTICE.to_string()); + assert_eq!(err.to_string(), CLI_PROMPT_PARKED_NOTICE); + assert!(err.to_string().contains("your prompt is saved")); + } + #[test] fn cli_prompt_fits_inline_caps_argv_agents_only() { // Positional/Flag are capped (single argv entry, Linux MAX_ARG_STRLEN). diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index c73dd44e89..370104c305 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -21,8 +21,8 @@ use executors::{ profile::{ExecutorConfig, ExecutorConfigs}, }; use local_deployment::pty::{ - PtyCommand, cli_prompt_fits_inline, cli_tmux_available, cli_tmux_session_exists, - cli_tmux_session_name, remove_cli_prompt_file, send_cli_keys, + CLI_PROMPT_PARKED_NOTICE, PtyCommand, cli_prompt_fits_inline, cli_tmux_available, + cli_tmux_session_exists, cli_tmux_session_name, remove_cli_prompt_file, send_cli_keys, }; use serde::{Deserialize, Serialize}; use sqlx::SqlitePool; @@ -454,11 +454,7 @@ async fn handle_terminal_ws( "CLI tmux session for workspace {} never came up; prompt left parked", workspace_id ); - let _ = send_error( - &mut socket, - "Failed to start the agent session — your prompt is saved and will be delivered on the next attach", - ) - .await; + let _ = send_error(&mut socket, CLI_PROMPT_PARKED_NOTICE).await; return; } } From 91781f1bfeb75340f22852c867756a9e1c42ec76 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 16:09:20 +0000 Subject: [PATCH 03/14] fix(cli-mode): plug PtyService leak, reap load-buffer child, test prompt routing Address three review nits in the long-prompt transport: - terminal.rs: the "session never came up" recovery branch returned without close_session, leaking the PtyService map entry (one per failed attach). - pty.rs paste_via_tmux_buffer: a failed stdin write returned without reaping the load-buffer child; kill+await it so it can't linger as a zombie. - Extract the baked-vs-deferred prompt routing into a pure route_initial_prompt helper and unit-test it (blank->None, small->Baked untrimmed, oversized-> Deferred trimmed, StdinPipe->Baked, Unsupported->Deferred). --- crates/local-deployment/src/pty.rs | 89 ++++++++++++++++++++++++++++ crates/server/src/routes/terminal.rs | 28 ++++----- 2 files changed, 103 insertions(+), 14 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 40f71c97fa..4056a353db 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -186,6 +186,47 @@ pub fn cli_prompt_fits_inline(prompt_arg: &CliPromptArg, byte_len: usize) -> boo } } +/// How a workspace's parked initial prompt should be delivered to the freshly +/// launched CLI agent — decided from the raw prompt and the agent's +/// `prompt_arg`. +#[derive(Debug, PartialEq, Eq)] +pub enum CliPromptRouting { + /// Nothing to deliver: no prompt was carried, or it was blank after trim. + None, + /// Small enough to bake into the launch command's temp-file transport. + /// Carries the ORIGINAL (untrimmed) text — the bootstrap's + /// [`cli_prompt_file_content`] step re-trims and applies the leading-dash + /// guard, so downstream behavior is byte-identical to passing it straight + /// through. + Baked(String), + /// Too large to pass as one argv entry, or an agent with no launch-time + /// prompt arg: deliver by paste ([`send_cli_keys`]) after the pane is + /// confirmed up. Carries the trimmed text (what actually gets pasted). + Deferred(String), +} + +/// Decide how to deliver a CLI-first workspace's initial prompt. Pure so the +/// baked-vs-deferred routing that gates the deferred-clear recovery path is unit +/// testable without a live tmux/socket. Blank prompts route to `None` (the +/// caller carries no prompt and clears nothing); anything that fits inline is +/// `Baked`, everything else `Deferred`. +pub fn route_initial_prompt( + initial_prompt: Option, + prompt_arg: &CliPromptArg, +) -> CliPromptRouting { + let Some(prompt) = initial_prompt else { + return CliPromptRouting::None; + }; + let trimmed = prompt.trim(); + if trimmed.is_empty() { + CliPromptRouting::None + } else if cli_prompt_fits_inline(prompt_arg, trimmed.len()) { + CliPromptRouting::Baked(prompt) + } else { + CliPromptRouting::Deferred(trimmed.to_string()) + } +} + /// The exact bytes to write to a workspace's CLI prompt file for `prompt_arg`, /// or `None` when the (trimmed) prompt is blank — mirroring the old in-command /// quoting semantics so small prompts behave identically: the leading-dash @@ -917,6 +958,12 @@ async fn paste_via_tmux_buffer(workspace_id: Uuid, target: &str, text: &str) -> if let Some(mut stdin) = child.stdin.take() { if stdin.write_all(text.as_bytes()).await.is_err() { + // Reap the load-buffer child before bailing: dropping the Child does + // not wait() it, so a failed stdin write would otherwise leave a + // lingering/zombie tmux process. Drop stdin first (EOF), then kill + // (which also awaits the exit). + drop(stdin); + let _ = child.kill().await; return false; } // Drop stdin (EOF) so load-buffer completes. @@ -1706,6 +1753,48 @@ mod tests { assert!(!cli_prompt_fits_inline(&CliPromptArg::Unsupported, 1)); } + #[test] + fn route_initial_prompt_bakes_defers_or_drops() { + // No prompt carried -> nothing to deliver, nothing to clear. + assert_eq!( + route_initial_prompt(None, &CliPromptArg::Positional), + CliPromptRouting::None + ); + // Blank-after-trim -> None (the empty-TUI case must clear nothing). + assert_eq!( + route_initial_prompt(Some(" \n\t".to_string()), &CliPromptArg::Positional), + CliPromptRouting::None + ); + + // Small prompt that fits inline -> Baked, carrying the ORIGINAL + // (untrimmed) text; the bootstrap re-trims + dash-guards downstream. + assert_eq!( + route_initial_prompt(Some(" hi there ".to_string()), &CliPromptArg::Positional), + CliPromptRouting::Baked(" hi there ".to_string()) + ); + + // Oversized Positional prompt -> Deferred (paste path), carrying the + // trimmed text that will actually be pasted. + let big = "x".repeat(MAX_INLINE_PROMPT_BYTES + 1); + assert_eq!( + route_initial_prompt(Some(format!(" {big} ")), &CliPromptArg::Positional), + CliPromptRouting::Deferred(big.clone()) + ); + + // StdinPipe has no argv ceiling, so even a huge prompt bakes in. + assert_eq!( + route_initial_prompt(Some(big.clone()), &CliPromptArg::StdinPipe), + CliPromptRouting::Baked(big) + ); + + // Unsupported agents have no launch-time transport -> always Deferred + // (delivered post-launch by paste), never dropped or baked. + assert_eq!( + route_initial_prompt(Some("hello".to_string()), &CliPromptArg::Unsupported), + CliPromptRouting::Deferred("hello".to_string()) + ); + } + #[test] fn cli_bootstrap_falls_back_to_continue_then_fresh() { // With nothing explicit to run: continue the cwd's latest conversation diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 370104c305..fa0ce06e8c 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -21,8 +21,9 @@ use executors::{ profile::{ExecutorConfig, ExecutorConfigs}, }; use local_deployment::pty::{ - CLI_PROMPT_PARKED_NOTICE, PtyCommand, cli_prompt_fits_inline, cli_tmux_available, - cli_tmux_session_exists, cli_tmux_session_name, remove_cli_prompt_file, send_cli_keys, + CLI_PROMPT_PARKED_NOTICE, CliPromptRouting, PtyCommand, cli_tmux_available, + cli_tmux_session_exists, cli_tmux_session_name, remove_cli_prompt_file, route_initial_prompt, + send_cli_keys, }; use serde::{Deserialize, Serialize}; use sqlx::SqlitePool; @@ -313,19 +314,13 @@ async fn terminal_ws( // after the pane is confirmed up (see handle_terminal_ws), so they're // never truncated and never silently lost. Either way the parked // prompt is cleared only after delivery is confirmed. - let baked_prompt = match initial_prompt { - Some(prompt) => { - let trimmed = prompt.trim(); - if trimmed.is_empty() { - None - } else if cli_prompt_fits_inline(&spec.prompt_arg, trimmed.len()) { - Some(prompt) - } else { - deferred_prompt = Some(trimmed.to_string()); - None - } + let baked_prompt = match route_initial_prompt(initial_prompt, &spec.prompt_arg) { + CliPromptRouting::None => None, + CliPromptRouting::Baked(prompt) => Some(prompt), + CliPromptRouting::Deferred(prompt) => { + deferred_prompt = Some(prompt); + None } - None => None, }; // Remember which session's prompt to clear once delivery is // confirmed (only when we actually carried a prompt). @@ -455,6 +450,11 @@ async fn handle_terminal_ws( workspace_id ); let _ = send_error(&mut socket, CLI_PROMPT_PARKED_NOTICE).await; + // Tear down the PtyService entry for the (already-dead) tmux client + // before bailing; otherwise this early return leaks the session map + // entry — one per failed attach — that the normal exit path below + // would have reaped via close_session. + let _ = deployment.pty().close_session(session_id).await; return; } } From d37f6425c4709b6c85c8c720d4e192c26a2d206e Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 20:33:42 +0000 Subject: [PATCH 04/14] fix(cli-mode): confirm prompt delivery before clearing; serialize racing first-attaches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Council round 1 (P0/P1) on the long-prompt transport: - Baked prompts: tmux session existence is not a delivery ack — a missing agent binary leaves a healthy-looking session while the command -v guard skips the launch arm, and the old flow then cleared the parked DB copy (prompt lost in an orphaned file). The bootstrap's rm-at-hand-off is now the acknowledgement: the route polls for the prompt file to disappear (<=4s) before clearing, and StdinPipe launches rm inside the producer group ({ cat f; rm -f -- f; } | agent) so pipes ack at consumption too, not at agent exit. Verified on a scratch tmux socket (file survives missing-binary pane; self-deletes at hand-off while the agent still runs). - Deferred (>100KB) prompts: a flat 750ms sleep could paste into the missing-binary fallback shell (prompt executed as shell input) or a still-booting TUI (canonical-mode truncation), then clear the DB copy. Delivery now waits for a non-shell process in the pane (tmux #{pane_pid} + ps/pgrep; #{pane_current_command} is useless here — it reports the outer default-shell for the pane's whole life, verified empirically), stable across two polls, bounded at ~10s; unready panes leave the prompt parked. - Racing first-attaches: both could pass the no-session-yet gate, restage the same prompt file (truncate can tear the winner's in-flight cat) or paste an oversized prompt twice. CliPromptDelivery is an in-process exclusive claim, taken before the peek and held through confirm-then-clear; the losing attach simply attaches without the prompt. - wait_for_cli_session widened to 10x250ms, and the session-never-came-up branch now runs kill_cli_tmux_session so a session that squeaks in after the window can't strand the parked prompt behind the session-exists gate. - Delivery confirmation moved to a background task so the terminal starts streaming immediately instead of blanking behind the polls. - Hygiene: prompt file removed when create_session fails after staging; cli-prompts/ dir created 0700; per-send sequence-suffixed tmux buffers plus delete-buffer when a paste fails; the Flag spec value is shell-quoted like every other emitted word. --- crates/local-deployment/src/pty.rs | 256 +++++++++++++++++++++++++-- crates/server/src/routes/terminal.rs | 201 +++++++++++++++------ 2 files changed, 397 insertions(+), 60 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 4056a353db..93610614fe 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -130,15 +130,22 @@ fn cli_bootstrap( format!(r#"vk_p="$(cat {qfile})"; rm -f -- {qfile}; {base} "$vk_p""#) } // Prompt as a flag value (e.g. gemini/copilot `-i ""`); a - // leading '-' is harmless after the flag. + // leading '-' is harmless after the flag. The flag is one of our + // own spec constants, but quote it anyway (like the program and + // base args) so it can never be more than a single command word. CliPromptArg::Flag(flag) => { - format!(r#"vk_p="$(cat {qfile})"; rm -f -- {qfile}; {base} {flag} "$vk_p""#) + let qflag = shell_single_quote(flag); + format!(r#"vk_p="$(cat {qfile})"; rm -f -- {qfile}; {base} {qflag} "$vk_p""#) } // Prompt piped on stdin (e.g. amp); the TUI stays interactive // because the tmux pane keeps stdout a TTY. No argv-length ceiling - // at all. The file already carries the trailing newline printf added. + // at all. The file already carries the trailing newline printf + // added. The `rm` runs inside the pipeline's producer — right after + // `cat` streams the file — so consumption is acknowledged (file + // gone) as soon as the prompt is handed off, not when the agent + // eventually exits. CliPromptArg::StdinPipe => { - format!("cat {qfile} | {base}; rm -f -- {qfile}") + format!("{{ cat {qfile}; rm -f -- {qfile}; }} | {base}") } // No CLI way to seed the prompt — start the TUI and rely on a // post-launch keystroke delivery (loop automation / send-keys). @@ -270,6 +277,18 @@ fn cli_prompt_file_path(workspace_id: Uuid) -> PathBuf { fn write_cli_prompt_file(workspace_id: Uuid, content: &str) -> std::io::Result { let path = cli_prompt_file_path(workspace_id); if let Some(dir) = path.parent() { + // Owner-only dir: the files inside are already 0600, but a 0700 dir + // also keeps prompt-file names (workspace ids + staging times) from + // being enumerable by other local users. + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + std::fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(dir)?; + } + #[cfg(not(unix))] std::fs::create_dir_all(dir)?; } #[cfg(unix)] @@ -298,6 +317,56 @@ pub fn remove_cli_prompt_file(workspace_id: Uuid) { let _ = std::fs::remove_file(cli_prompt_file_path(workspace_id)); } +/// Whether a workspace's staged CLI prompt file is still on disk. The bootstrap +/// `rm`s the file the moment it consumes it, so "file gone" is the delivery +/// acknowledgement the terminal route polls before clearing the parked DB copy. +/// A launch that never consumed the prompt (agent binary missing → the +/// `command -v` guard skips the whole launch arm) leaves the file — and +/// therefore the DB copy — in place for the next fresh session. +pub fn cli_prompt_file_exists(workspace_id: Uuid) -> bool { + cli_prompt_file_path(workspace_id).exists() +} + +/// Workspaces with a CLI prompt delivery currently in flight. See +/// [`CliPromptDelivery`]. +static CLI_PROMPT_DELIVERIES: OnceLock>> = OnceLock::new(); + +/// In-process claim that exactly ONE terminal attach delivers a workspace's +/// parked CLI prompt at a time. Two racing first-attaches can both pass the +/// "no tmux session yet" gate before either spawns; without the claim both +/// would stage the same prompt file (the loser's `truncate` can tear the +/// winner's in-flight `cat`) or, for an oversized prompt, both would paste it +/// (double delivery). The loser simply attaches without carrying the prompt — +/// `new-session -A` ignores its bootstrap anyway. Dropping the claim releases +/// it; delivery holders keep it until the parked prompt is cleared or +/// explicitly left parked. +#[derive(Debug)] +pub struct CliPromptDelivery(Uuid); + +impl CliPromptDelivery { + /// Claim the workspace's prompt delivery, or `None` if another attach in + /// this process already holds it. + pub fn try_claim(workspace_id: Uuid) -> Option { + let set = CLI_PROMPT_DELIVERIES.get_or_init(Default::default); + // Release the registry guard BEFORE constructing the claim: an eagerly + // built `Self` on the not-inserted path (`then_some`) would be dropped + // while the guard is still alive, and `Drop` re-locks the same + // (non-reentrant) mutex — a self-deadlock. + let inserted = set.lock().ok()?.insert(workspace_id); + inserted.then(|| Self(workspace_id)) + } +} + +impl Drop for CliPromptDelivery { + fn drop(&mut self) { + if let Some(set) = CLI_PROMPT_DELIVERIES.get() + && let Ok(mut set) = set.lock() + { + set.remove(&self.0); + } + } +} + /// How to install each interactive-CLI agent, shown in the pane when its binary /// isn't on PATH. Kept here (next to the bootstrap that prints it) rather than on /// the spec so the message stays a deployment concern. @@ -804,6 +873,117 @@ pub fn cli_tmux_available() -> bool { tmux_available() } +/// Whether the AGENT (a non-shell process) currently owns a workspace's CLI +/// pane. `None` when the pane itself can't be read (session gone). +/// +/// tmux runs our bootstrap string via `default-shell -c`, and every launch +/// stage shares that shell's process group — so `#{pane_current_command}` +/// reports the outer shell for the pane's whole life (verified empirically) +/// and can't distinguish "agent running" from "fallback shell". Instead this +/// inspects the pane's process tree: the pane root (`#{pane_pid}`) or one of +/// its direct children being a non-shell process means the agent is up. The +/// bootstrap keeps the agent a direct child of the pane shell (it is not +/// `exec`ed), and the missing-binary fallback `exec`s an interactive shell +/// with no children — both shapes verified on a scratch socket. +pub async fn cli_pane_agent_running(workspace_id: Uuid) -> Option { + if !tmux_available() { + return None; + } + let target = cli_tmux_session_name(workspace_id); + let output = tokio::process::Command::new("tmux") + .args([ + "-L", + CLI_TMUX_SOCKET, + "list-panes", + "-t", + &target, + "-F", + "#{pane_pid}", + ]) + .stderr(std::process::Stdio::null()) + .output() + .await + .ok()?; + if !output.status.success() { + return None; + } + let pane_pid = String::from_utf8_lossy(&output.stdout) + .lines() + .next()? + .trim() + .parse::() + .ok()?; + + // Pane root: normally the bootstrap/fallback shell, but a future exec'd + // agent would show up here directly. + if let Some(comm) = process_comm(pane_pid).await + && !is_shell_command(&comm) + { + return Some(true); + } + + // Direct children of the pane shell (`pgrep -l -P` prints " "; + // a non-zero exit just means no children — the bootstrap is between + // stages or the fallback shell is idle). + let children = tokio::process::Command::new("pgrep") + .args(["-l", "-P", &pane_pid.to_string()]) + .stderr(std::process::Stdio::null()) + .output() + .await + .ok()?; + let listing = String::from_utf8_lossy(&children.stdout); + Some( + listing + .lines() + .filter_map(|line| line.split_whitespace().nth(1)) + .any(|comm| !is_shell_command(comm)), + ) +} + +/// Best-effort process name for a pid (`ps -o comm=`), normalized. +async fn process_comm(pid: u32) -> Option { + let output = tokio::process::Command::new("ps") + .args(["-o", "comm=", "-p", &pid.to_string()]) + .stderr(std::process::Stdio::null()) + .output() + .await + .ok()?; + if !output.status.success() { + return None; + } + let comm = normalize_comm(String::from_utf8_lossy(&output.stdout).trim()).to_string(); + (!comm.is_empty()).then_some(comm) +} + +/// Normalize a `ps`/`pgrep` process name for shell detection: login shells +/// report as `-zsh`, and macOS `ps -o comm=` can report a full path. +fn normalize_comm(comm: &str) -> &str { + let comm = comm.trim().trim_start_matches('-'); + comm.rsplit('/').next().unwrap_or(comm) +} + +/// Whether a process name is a shell — i.e. NOT the agent. Used to gate the +/// deferred paste: pasting into a shell would execute/mangle the prompt as +/// shell input (the bootstrap still starting up, or the missing-binary +/// fallback shell), so delivery waits for a non-shell process in the pane. +pub fn is_shell_command(cmd: &str) -> bool { + matches!( + normalize_comm(cmd), + "sh" | "bash" + | "zsh" + | "dash" + | "ash" + | "ksh" + | "mksh" + | "csh" + | "tcsh" + | "fish" + | "nu" + | "busybox" + | "login" + ) +} + /// Inverse of [`cli_tmux_session_name`]: recover the workspace id from one of /// our tmux session names. Returns `None` for anything outside the `vk_` /// namespace (e.g. a user-created session on the same socket). @@ -943,7 +1123,13 @@ pub async fn send_cli_keys(workspace_id: Uuid, text: &str) -> bool { async fn paste_via_tmux_buffer(workspace_id: Uuid, target: &str, text: &str) -> bool { use tokio::io::AsyncWriteExt; - let buffer = format!("vk_prompt_{}", workspace_id.simple()); + // Per-send sequence number on top of the workspace namespace: two + // concurrent sends to the SAME workspace (e.g. a loop wake-up racing a + // deferred initial-prompt delivery) must not overwrite each other's buffer + // between load and paste. + static SEND_SEQ: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let seq = SEND_SEQ.fetch_add(1, Ordering::Relaxed); + let buffer = format!("vk_prompt_{}_{seq}", workspace_id.simple()); let mut child = match tokio::process::Command::new("tmux") .args(["-L", CLI_TMUX_SOCKET, "load-buffer", "-b", &buffer, "-"]) @@ -976,7 +1162,7 @@ async fn paste_via_tmux_buffer(workspace_id: Uuid, target: &str, text: &str) -> return false; } - tmux_ok(&[ + let pasted = tmux_ok(&[ "-L", CLI_TMUX_SOCKET, "paste-buffer", @@ -987,7 +1173,13 @@ async fn paste_via_tmux_buffer(workspace_id: Uuid, target: &str, text: &str) -> "-t", target, ]) - .await + .await; + if !pasted { + // `-d` only fires on a successful paste; don't leave the staged prompt + // readable in the server-global buffer list after a failed one. + tmux_ok(&["-L", CLI_TMUX_SOCKET, "delete-buffer", "-b", &buffer]).await; + } + pasted } /// Seconds since the Unix epoch (best-effort; 0 if the system clock is before @@ -1656,16 +1848,20 @@ mod tests { fn cli_bootstrap_flag_and_stdin_prompt_forms_read_from_file() { let file = Path::new("/tmp/vk/p.txt"); - // Flag agents expand the file into the flag's value, double-quoted. + // Flag agents expand the file into the flag's value, double-quoted; + // the flag itself is quoted like every other word we emit. let flag_spec = CliLaunchSpec::new("gemini", vec![]) .with_prompt_arg(CliPromptArg::Flag("-i".to_string())); let b = cli_bootstrap(&flag_spec, None, Some(file)); - assert!(b.contains("rm -f -- '/tmp/vk/p.txt'; 'gemini' -i \"$vk_p\"")); + assert!(b.contains("rm -f -- '/tmp/vk/p.txt'; 'gemini' '-i' \"$vk_p\"")); // StdinPipe agents pipe the file into the program — no argv ceiling. + // The `rm` runs inside the producer group, right after `cat` streams + // the file, so consumption is acknowledged (file gone) immediately — + // not when the agent eventually exits. let pipe_spec = CliLaunchSpec::new("amp", vec![]).with_prompt_arg(CliPromptArg::StdinPipe); let b = cli_bootstrap(&pipe_spec, None, Some(file)); - assert!(b.contains("cat '/tmp/vk/p.txt' | 'amp'; rm -f -- '/tmp/vk/p.txt'")); + assert!(b.contains("{ cat '/tmp/vk/p.txt'; rm -f -- '/tmp/vk/p.txt'; } | 'amp'")); } #[test] @@ -1795,6 +1991,46 @@ mod tests { ); } + #[test] + fn cli_prompt_delivery_claim_is_exclusive_until_dropped() { + // Fresh workspace id so parallel tests can't collide in the global set. + let wid = Uuid::new_v4(); + let claim = CliPromptDelivery::try_claim(wid).expect("first claim succeeds"); + // A racing second attach must NOT also carry the prompt. + assert!( + CliPromptDelivery::try_claim(wid).is_none(), + "second claim while held must fail" + ); + // Another workspace's delivery is independent. + let other = Uuid::new_v4(); + assert!(CliPromptDelivery::try_claim(other).is_some()); + // Releasing (drop) lets the next attach retry delivery. + drop(claim); + assert!( + CliPromptDelivery::try_claim(wid).is_some(), + "claim must be reusable after drop" + ); + } + + #[test] + fn shell_commands_are_recognized_for_paste_gating() { + // The deferred paste must never target a pane whose process tree is + // all shells (bootstrap still starting, or the missing-binary + // fallback shell). + for shell in ["sh", "bash", "zsh", "dash", "fish"] { + assert!(is_shell_command(shell), "{shell} must gate the paste"); + } + // Login-shell (`-zsh`) and full-path (`/bin/zsh`, macOS ps) spellings + // normalize to the same shell names. + assert!(is_shell_command("-zsh")); + assert!(is_shell_command("/bin/bash")); + assert!(is_shell_command(" -/usr/bin/fish".trim())); + // Agent TUIs (binary or interpreter names) unblock the paste. + for agent in ["claude", "codex", "node", "amp", "gemini"] { + assert!(!is_shell_command(agent), "{agent} must allow the paste"); + } + } + #[test] fn cli_bootstrap_falls_back_to_continue_then_fresh() { // With nothing explicit to run: continue the cwd's latest conversation diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index fa0ce06e8c..63521bd50a 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -21,8 +21,9 @@ use executors::{ profile::{ExecutorConfig, ExecutorConfigs}, }; use local_deployment::pty::{ - CLI_PROMPT_PARKED_NOTICE, CliPromptRouting, PtyCommand, cli_tmux_available, - cli_tmux_session_exists, cli_tmux_session_name, remove_cli_prompt_file, route_initial_prompt, + CLI_PROMPT_PARKED_NOTICE, CliPromptDelivery, CliPromptRouting, PtyCommand, + cli_pane_agent_running, cli_prompt_file_exists, cli_tmux_available, cli_tmux_session_exists, + cli_tmux_session_name, kill_cli_tmux_session, remove_cli_prompt_file, route_initial_prompt, send_cli_keys, }; use serde::{Deserialize, Serialize}; @@ -193,6 +194,11 @@ async fn terminal_ws( // delivered by paste AFTER the pane is up, rather than baked into the launch // command. Set inside the Cli arm; delivered in handle_terminal_ws. let mut deferred_prompt: Option = None; + // Exclusive in-process claim on this workspace's prompt delivery — held + // whenever this attach carries the prompt, released once the parked copy is + // cleared or deliberately left parked. Racing first-attaches that lose the + // claim attach without the prompt instead of double-staging/double-pasting. + let mut delivery_claim: Option = None; let (working_dir, command) = match query.mode { TerminalMode::Cli => { @@ -286,16 +292,39 @@ async fn terminal_ws( && cli_tmux_available() && !cli_tmux_session_exists(query.workspace_id).await => { - match Session::peek_pending_cli_prompt(pool, s.id).await { - Ok(prompt) => prompt, - Err(e) => { - // The prompt is not lost — it stays parked and the - // next attach re-peeks — but a transient DB error - // here delays delivery, so make it observable. - tracing::warn!( - "Failed to read pending CLI prompt for session {}: {}", - s.id, - e + // Two attaches can both pass the no-session-yet gate before + // either spawns; only the claim holder carries the prompt + // (the loser's `-A` attach ignores its bootstrap anyway), + // so the prompt file can't be re-truncated mid-`cat` and an + // oversized prompt can't be pasted twice. + match CliPromptDelivery::try_claim(query.workspace_id) { + Some(claim) => { + match Session::peek_pending_cli_prompt(pool, s.id).await { + Ok(Some(prompt)) => { + delivery_claim = Some(claim); + Some(prompt) + } + // Nothing parked: release the claim (drop). + Ok(None) => None, + Err(e) => { + // The prompt is not lost — it stays parked + // and the next attach re-peeks — but a + // transient DB error here delays delivery, + // so make it observable. + tracing::warn!( + "Failed to read pending CLI prompt for session {}: {}", + s.id, + e + ); + None + } + } + } + None => { + tracing::debug!( + "CLI prompt delivery for workspace {} already in flight; \ + attaching without the prompt", + query.workspace_id ); None } @@ -326,6 +355,10 @@ async fn terminal_ws( // confirmed (only when we actually carried a prompt). if baked_prompt.is_some() || deferred_prompt.is_some() { prompt_session_to_clear = session.as_ref().map(|s| s.id); + } else { + // Blank-after-trim prompt: nothing will be delivered, so don't + // hold the delivery claim across the attach. + delivery_claim = None; } ( @@ -375,6 +408,7 @@ async fn terminal_ws( query.workspace_id, prompt_session_to_clear, deferred_prompt, + delivery_claim, ) })) } @@ -390,6 +424,7 @@ async fn handle_terminal_ws( workspace_id: Uuid, prompt_session_to_clear: Option, deferred_prompt: Option, + delivery_claim: Option, ) { let (session_id, mut output_rx) = match deployment .pty() @@ -399,6 +434,12 @@ async fn handle_terminal_ws( Ok(result) => result, Err(e) => { tracing::error!("Failed to create PTY session: {}", e); + // A prompt file staged before the spawn failure must not outlive + // it (the DB copy stays parked for the next attach; the claim is + // released on return). + if prompt_session_to_clear.is_some() { + remove_cli_prompt_file(workspace_id); + } let _ = send_error(&mut socket, &e.to_string()).await; return; } @@ -407,44 +448,50 @@ async fn handle_terminal_ws( // create_session returning Ok only means the tmux *client* process spawned; // the tmux server can still reject the command moments later (historically: // an over-long prompt baked into `new-session`). So confirm the session - // actually exists before clearing the parked prompt — and, for a deferred - // large prompt, before pasting it in. A session that never comes up leaves - // the prompt saved for the next attach and tells the user so, instead of - // silently destroying it. + // actually exists before doing anything with the parked prompt. A session + // that never comes up leaves the prompt saved for the next attach and tells + // the user so, instead of silently destroying it. if let Some(clear_session_id) = prompt_session_to_clear { if wait_for_cli_session(workspace_id).await { - let delivered = match &deferred_prompt { - Some(text) => { - // Give the freshly-launched TUI a moment to be ready to - // accept a paste, then deliver the oversized prompt via the - // buffer path (no argv-length ceiling). - tokio::time::sleep(std::time::Duration::from_millis(750)).await; - send_cli_keys(workspace_id, text).await - } - None => true, - }; - if delivered { - if let Err(e) = - Session::clear_pending_cli_prompt(&deployment.db().pool, clear_session_id).await - { + // Confirm delivery and clear the parked prompt in the background so + // the pane's output starts streaming immediately — the confirmation + // polls below would otherwise hold the whole terminal blank. + let pool = deployment.db().pool.clone(); + tokio::spawn(async move { + // Hold the exclusive delivery claim for the whole + // confirm-then-clear window. + let _claim = delivery_claim; + let delivered = match deferred_prompt { + Some(text) => deliver_deferred_prompt(workspace_id, &text).await, + None => confirm_baked_prompt_consumed(workspace_id).await, + }; + if delivered { + if let Err(e) = Session::clear_pending_cli_prompt(&pool, clear_session_id).await + { + tracing::warn!( + "Failed to clear delivered CLI prompt for session {}: {}", + clear_session_id, + e + ); + } + } else { + // Leave the prompt parked; a later fresh session delivers + // it (delivery is only ever confirmed, never assumed). tracing::warn!( - "Failed to clear delivered CLI prompt for session {}: {}", - clear_session_id, - e + "CLI prompt delivery for workspace {} unconfirmed; left parked", + workspace_id ); } - } else { - // Leave the prompt parked; the next attach retries delivery. - tracing::warn!( - "Failed to deliver large CLI prompt to workspace {}; left parked", - workspace_id - ); - } + }); } else { - // The session never appeared — remove the transient prompt file (the - // DB copy stays parked) and surface the failure. The frontend renders - // this in red and halts its reconnect loop. - remove_cli_prompt_file(workspace_id); + // The session never appeared — tear down whatever half-state the + // launch left behind and surface the failure. `kill_cli_tmux_session` + // both removes the transient prompt file (the DB copy stays parked) + // and kills a session that squeaked in after the poll window, so the + // next attach deterministically re-peeks the parked prompt instead + // of finding a stray session that blocks delivery. The frontend + // renders the error in red and halts its reconnect loop. + kill_cli_tmux_session(workspace_id).await; tracing::error!( "CLI tmux session for workspace {} never came up; prompt left parked", workspace_id @@ -514,17 +561,71 @@ async fn handle_terminal_ws( /// Poll for the workspace's CLI tmux session to appear after a spawn. The /// session existing proves tmux accepted `new-session` and the bootstrap (which -/// owns the prompt file) is running; only then is it safe to clear the parked -/// prompt. Short backoff (~5 × 200ms) — the session shows up immediately when -/// tmux accepts the command, so this only spins when it's failing. +/// owns the prompt file) is running. Bounded backoff (~10 × 250ms): the session +/// shows up near-instantly when tmux accepts the command, so the full window is +/// only spent when the launch is failing — and a window that's too short would +/// false-negative on a heavily loaded machine, stranding a prompt that WAS +/// delivered (the replay hazard the deferred clear exists to prevent). async fn wait_for_cli_session(workspace_id: Uuid) -> bool { - for attempt in 0..5 { + for attempt in 0..10 { if cli_tmux_session_exists(workspace_id).await { return true; } - if attempt < 4 { - tokio::time::sleep(std::time::Duration::from_millis(200)).await; + if attempt < 9 { + tokio::time::sleep(std::time::Duration::from_millis(250)).await; + } + } + false +} + +/// Confirm the pane's bootstrap actually consumed the staged (baked) prompt +/// file. The bootstrap `rm`s the file the moment it hands the prompt to the +/// agent, so "file gone" is a real delivery acknowledgement — while a launch +/// that never consumed it (agent binary missing → the bootstrap's `command -v` +/// guard skips the launch arm and drops to the fallback shell) leaves the file +/// behind, and the parked DB prompt must then survive for a later fresh +/// session instead of being cleared into the void. +async fn confirm_baked_prompt_consumed(workspace_id: Uuid) -> bool { + for _ in 0..20 { + if !cli_prompt_file_exists(workspace_id) { + return true; + } + tokio::time::sleep(std::time::Duration::from_millis(200)).await; + } + false +} + +/// Deliver an oversized (non-inlineable) prompt by paste once the AGENT owns +/// the pane. Pasting earlier would hand the prompt to the bootstrap/fallback +/// shell instead — which executes it as shell input (binary missing) or +/// truncates it at the tty's canonical-mode line limit (TUI not yet in raw +/// mode). Readiness = a non-shell process in the pane on two consecutive +/// polls (a single read could catch the short-lived first leg of the +/// `--continue || fresh` relaunch), plus a short grace for the TUI to enter +/// raw mode. Bounded at ~10s; an unready pane leaves the prompt parked. +async fn deliver_deferred_prompt(workspace_id: Uuid, text: &str) -> bool { + let mut stable = 0u32; + for _ in 0..40 { + match cli_pane_agent_running(workspace_id).await { + Some(true) => { + stable += 1; + if stable >= 2 { + tokio::time::sleep(std::time::Duration::from_millis(750)).await; + return send_cli_keys(workspace_id, text).await; + } + } + Some(false) => stable = 0, + None => { + // Pane unreadable: if the session is really gone, give up and + // leave the prompt parked; a transient probe failure (ps/pgrep + // hiccup) just resets the stability counter. + if !cli_tmux_session_exists(workspace_id).await { + return false; + } + stable = 0; + } } + tokio::time::sleep(std::time::Duration::from_millis(250)).await; } false } From 1f663b3a2457519686c20590945540466c5395ca Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 21:13:59 +0000 Subject: [PATCH 05/14] fix(cli-mode): CAS prompt clear, live-session/post-resume paste recovery, simplify pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex xhigh review fixes: - clear_pending_cli_prompt is now a compare-and-swap on the exact delivered value: a newer prompt parked mid-confirmation (loop wake-up re-parked while an initial prompt was being confirmed) can no longer be destroyed by the older delivery's clear. Superseded clears log and leave it parked. - A parked prompt behind a LIVE tmux session or a resume launch is no longer stranded: attaches now deliver it by paste into the running agent (readiness gate + claim + confirmed clear), so an unconfirmed earlier delivery or a re-parked wake-up retries on every attach instead of waiting for the session to die — and resume precedence no longer permanently blocks re-parked wake-up prompts (they arrive as follow-ups after the resume launch). - The baked-prompt ack now requires the staged file to be gone AND a non-shell process owning the pane, closing the consumed-but-never-executed window (agent exec fails after command -v passes) that file-gone alone would have mistaken for delivery. Simplify pass (behavior-preserving): - PtyCommand::TmuxCli carries workspace_id instead of a session name that was reverse-parsed back into one (impossible-error branch deleted). - PromptDelivery bundles workspace/clear-session/peeked/deferred/claim so a claim without a prompt is unrepresentable; too_many_arguments allow dropped. - PtyError::PromptStageFailed is a unit variant whose display IS the shared recovery notice; active_resume_id() is the single owner of the resume-wins predicate for both the staging gate and the bootstrap; the read-and-delete bootstrap stage is emitted once for Positional/Flag; route_initial_prompt always carries trimmed text; loop_supervisor re-park has one owner (repark_prompt, now logging its failures); cli_tmux_session_exists reuses tmux_ok; the pane probe runs pgrep before ps and is_shell_command owns comm normalization; internal helpers dropped from the crate's public surface. --- ...2d0357608d2b0be2eec0e6201e6d193a2dff.json} | 6 +- crates/db/src/models/session.rs | 42 +-- .../local-deployment/src/loop_supervisor.rs | 31 ++- crates/local-deployment/src/pty.rs | 209 ++++++++------- crates/server/src/routes/terminal.rs | 249 ++++++++++-------- 5 files changed, 301 insertions(+), 236 deletions(-) rename crates/db/.sqlx/{query-abf14b3dafc2b8538be32597cc196a8a8c6de837f4073ed50b6d93d9eb8928d8.json => query-b4f9943e22df65355ecd158ed92f2d0357608d2b0be2eec0e6201e6d193a2dff.json} (50%) diff --git a/crates/db/.sqlx/query-abf14b3dafc2b8538be32597cc196a8a8c6de837f4073ed50b6d93d9eb8928d8.json b/crates/db/.sqlx/query-b4f9943e22df65355ecd158ed92f2d0357608d2b0be2eec0e6201e6d193a2dff.json similarity index 50% rename from crates/db/.sqlx/query-abf14b3dafc2b8538be32597cc196a8a8c6de837f4073ed50b6d93d9eb8928d8.json rename to crates/db/.sqlx/query-b4f9943e22df65355ecd158ed92f2d0357608d2b0be2eec0e6201e6d193a2dff.json index d41ecebfae..6b3c308113 100644 --- a/crates/db/.sqlx/query-abf14b3dafc2b8538be32597cc196a8a8c6de837f4073ed50b6d93d9eb8928d8.json +++ b/crates/db/.sqlx/query-b4f9943e22df65355ecd158ed92f2d0357608d2b0be2eec0e6201e6d193a2dff.json @@ -1,12 +1,12 @@ { "db_name": "SQLite", - "query": "UPDATE sessions\n SET pending_cli_prompt = NULL\n WHERE id = $1 AND pending_cli_prompt IS NOT NULL", + "query": "UPDATE sessions\n SET pending_cli_prompt = NULL\n WHERE id = $1 AND pending_cli_prompt = $2", "describe": { "columns": [], "parameters": { - "Right": 1 + "Right": 2 }, "nullable": [] }, - "hash": "abf14b3dafc2b8538be32597cc196a8a8c6de837f4073ed50b6d93d9eb8928d8" + "hash": "b4f9943e22df65355ecd158ed92f2d0357608d2b0be2eec0e6201e6d193a2dff" } diff --git a/crates/db/src/models/session.rs b/crates/db/src/models/session.rs index b109dddefb..16d71e963a 100644 --- a/crates/db/src/models/session.rs +++ b/crates/db/src/models/session.rs @@ -211,13 +211,14 @@ impl Session { } /// Read the parked CLI prompt WITHOUT clearing it. The clear is deferred - /// to [`clear_pending_cli_prompt`] until the tmux session is confirmed to - /// *exist* (and, for a large paste-delivered prompt, until the paste - /// succeeds), so neither a failure between attach and spawn nor tmux - /// rejecting the launch command after spawn can destroy the user's only - /// copy of their prompt. Two racing first-attaches that both peek the same - /// prompt are harmless — whichever wins `new-session` carries it (the - /// loser's `-A` reattach ignores its bootstrap). + /// to [`clear_pending_cli_prompt`] until delivery is CONFIRMED — the + /// launch bootstrap consumed the staged prompt file with the agent up, or + /// the paste into the agent's pane succeeded — so neither a failure + /// between attach and spawn nor tmux rejecting the launch command after + /// spawn can destroy the user's only copy of the prompt. Racing + /// first-attaches are serialized by an in-process delivery claim + /// (`CliPromptDelivery` in local-deployment); the losing attach simply + /// doesn't carry the prompt. pub async fn peek_pending_cli_prompt( pool: &SqlitePool, id: Uuid, @@ -231,19 +232,30 @@ impl Session { .flatten()) } - /// Clear the parked CLI prompt once it has been delivered to a freshly - /// created tmux session. Idempotent and atomic (a single guarded UPDATE), - /// so a double-call from racing attaches is a no-op. - pub async fn clear_pending_cli_prompt(pool: &SqlitePool, id: Uuid) -> Result<(), sqlx::Error> { - sqlx::query!( + /// Clear the parked CLI prompt once THIS delivery's copy of it has been + /// confirmed delivered. Compare-and-swap on the exact delivered value — + /// a prompt parked mid-confirmation (e.g. a loop wake-up re-parked while + /// an initial prompt's delivery was still being confirmed) is newer, + /// undelivered, and must not be destroyed by the older delivery's clear. + /// Returns whether the clear happened (`false` = superseded; the newer + /// prompt stays parked for its own delivery). Idempotent and atomic (a + /// single guarded UPDATE), so a double-call from racing attaches is a + /// no-op. + pub async fn clear_pending_cli_prompt( + pool: &SqlitePool, + id: Uuid, + delivered: &str, + ) -> Result { + let result = sqlx::query!( r#"UPDATE sessions SET pending_cli_prompt = NULL - WHERE id = $1 AND pending_cli_prompt IS NOT NULL"#, - id + WHERE id = $1 AND pending_cli_prompt = $2"#, + id, + delivered ) .execute(pool) .await?; - Ok(()) + Ok(result.rows_affected() > 0) } /// Persist the model + reasoning effort chosen at CLI-first creation so the diff --git a/crates/local-deployment/src/loop_supervisor.rs b/crates/local-deployment/src/loop_supervisor.rs index b9392ddee8..c36760acf2 100644 --- a/crates/local-deployment/src/loop_supervisor.rs +++ b/crates/local-deployment/src/loop_supervisor.rs @@ -26,6 +26,7 @@ use db::{ session::Session, }, }; +use uuid::Uuid; use crate::pty::{capture_cli_pane, cli_tmux_available, cli_tmux_session_exists, send_cli_keys}; @@ -413,10 +414,8 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), // The tmux session may have been reaped before a long usage-window wake // came due — re-park the prompt so the next attach delivers it. if !cli_tmux_session_exists(wid).await { - if let Ok(Some(session)) = Session::find_latest_by_workspace_id(pool, wid).await { - let _ = Session::set_pending_cli_prompt(pool, session.id, &prompt).await; - tracing::info!("loop: workspace {wid} session gone; re-parked wake-up prompt"); - } + repark_prompt(pool, wid, &prompt).await; + tracing::info!("loop: workspace {wid} session gone; re-parked wake-up prompt"); ScheduledWakeup::mark_fired(pool, wakeup.id).await?; continue; } @@ -457,15 +456,33 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), // prompt so the next terminal attach delivers it rather than dropping // the wake-up — mirrors the session-gone branch above. tracing::warn!("loop: failed to deliver wake-up to workspace {wid}; re-parking"); - if let Ok(Some(session)) = Session::find_latest_by_workspace_id(pool, wid).await { - let _ = Session::set_pending_cli_prompt(pool, session.id, &prompt).await; - } + repark_prompt(pool, wid, &prompt).await; } ScheduledWakeup::mark_fired(pool, wakeup.id).await?; } Ok(()) } +/// Best-effort: park `prompt` on the workspace's latest session so a terminal +/// attach delivers it (the attach path pastes a parked prompt into a live +/// agent pane, or hands it to the next fresh launch). Shared by both wake-up +/// failure branches so the re-park policy has exactly one owner. +async fn repark_prompt(pool: &sqlx::SqlitePool, wid: Uuid, prompt: &str) { + match Session::find_latest_by_workspace_id(pool, wid).await { + Ok(Some(session)) => { + if let Err(e) = Session::set_pending_cli_prompt(pool, session.id, prompt).await { + tracing::warn!("loop: failed to re-park wake-up prompt for workspace {wid}: {e}"); + } + } + Ok(None) => { + tracing::warn!("loop: no session to re-park wake-up prompt for workspace {wid}") + } + Err(e) => { + tracing::warn!("loop: failed to re-park wake-up prompt for workspace {wid}: {e}") + } + } +} + /// Scan each enabled workspace's pane and schedule a wake-up on a fresh limit. async fn detect_and_schedule(db: &DBService, now: DateTime) -> Result<(), sqlx::Error> { let pool = &db.pool; diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 93610614fe..e7a0b91b9d 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -30,7 +30,10 @@ pub enum PtyCommand { /// session (and whatever runs inside it) survives WebSocket disconnects /// and server restarts; reconnects reattach instead of respawning. TmuxCli { - session_name: String, + /// The workspace this pane belongs to; the tmux session name and the + /// staged prompt-file path are both derived from it + /// ([`cli_tmux_session_name`], [`write_cli_prompt_file`]). + workspace_id: Uuid, /// claude's own session UUID to resume (the workspace's selected uix /// chat). When set, the bootstrap runs `claude --resume ` so CLI /// mode joins the *exact* conversation the chat UI is showing, and @@ -100,10 +103,7 @@ fn cli_bootstrap( CliContinue::Fresh => base.clone(), }; - // Only a strict UUID may be interpolated into the shell string. Agent - // session ids are UUIDs, so this both validates intent and forecloses - // shell injection via the id. - let launch = if let Some(id) = resume_session_id.filter(|id| is_uuid(id)) { + let launch = if let Some(id) = active_resume_id(resume_session_id) { match &spec.resume { // ` --resume ` — flags still apply (claude). CliResume::Flag(flag) => format!("{base} {flag} {id}"), @@ -121,13 +121,17 @@ fn cli_bootstrap( // ever expands inside double quotes, so it can't be word-split or parsed // as shell. The file self-deletes (`rm`) once consumed. let qfile = shell_single_quote(&file.to_string_lossy()); + // Shared read-and-delete stage for the argv-passing arms: consume the + // file into `vk_p`, then delete it (the delete doubles as the delivery + // acknowledgement — see [`cli_prompt_file_exists`]). + let read_rm = format!(r#"vk_p="$(cat {qfile})"; rm -f -- {qfile};"#); match &spec.prompt_arg { // Trailing positional arg. The leading-dash guard and any trailing // whitespace handling are baked into the file's contents // ([`cli_prompt_file_content`]); command substitution strips a // trailing newline, which is harmless. CliPromptArg::Positional => { - format!(r#"vk_p="$(cat {qfile})"; rm -f -- {qfile}; {base} "$vk_p""#) + format!(r#"{read_rm} {base} "$vk_p""#) } // Prompt as a flag value (e.g. gemini/copilot `-i ""`); a // leading '-' is harmless after the flag. The flag is one of our @@ -135,7 +139,7 @@ fn cli_bootstrap( // base args) so it can never be more than a single command word. CliPromptArg::Flag(flag) => { let qflag = shell_single_quote(flag); - format!(r#"vk_p="$(cat {qfile})"; rm -f -- {qfile}; {base} {qflag} "$vk_p""#) + format!(r#"{read_rm} {base} {qflag} "$vk_p""#) } // Prompt piped on stdin (e.g. amp); the TUI stays interactive // because the tmux pane keeps stdout a TTY. No argv-length ceiling @@ -180,12 +184,21 @@ fn cli_bootstrap( /// post-launch by paste instead (see [`cli_prompt_fits_inline`]). const MAX_INLINE_PROMPT_BYTES: usize = 100_000; +/// The resume id that will actually drive a resume launch: only a strict UUID +/// may ever be interpolated into the bootstrap shell string (agent session ids +/// are UUIDs, so this both validates intent and forecloses shell injection via +/// the id). The prompt-staging gate uses the SAME predicate, so a file is +/// staged exactly when the bootstrap will consume it — the two can't drift. +fn active_resume_id(resume_session_id: Option<&str>) -> Option<&str> { + resume_session_id.filter(|id| is_uuid(id)) +} + /// Whether an initial prompt of `byte_len` bytes can be baked into the launch /// command for an agent with this `prompt_arg`, or must be delivered after the /// TUI is up (via [`send_cli_keys`]). `StdinPipe` has no argv ceiling; /// `Positional`/`Flag` pass the prompt as one argv entry and are capped; /// `Unsupported` has no launch-time transport at all. -pub fn cli_prompt_fits_inline(prompt_arg: &CliPromptArg, byte_len: usize) -> bool { +fn cli_prompt_fits_inline(prompt_arg: &CliPromptArg, byte_len: usize) -> bool { match prompt_arg { CliPromptArg::Positional | CliPromptArg::Flag(_) => byte_len <= MAX_INLINE_PROMPT_BYTES, CliPromptArg::StdinPipe => true, @@ -201,14 +214,10 @@ pub enum CliPromptRouting { /// Nothing to deliver: no prompt was carried, or it was blank after trim. None, /// Small enough to bake into the launch command's temp-file transport. - /// Carries the ORIGINAL (untrimmed) text — the bootstrap's - /// [`cli_prompt_file_content`] step re-trims and applies the leading-dash - /// guard, so downstream behavior is byte-identical to passing it straight - /// through. Baked(String), /// Too large to pass as one argv entry, or an agent with no launch-time /// prompt arg: deliver by paste ([`send_cli_keys`]) after the pane is - /// confirmed up. Carries the trimmed text (what actually gets pasted). + /// confirmed up. Deferred(String), } @@ -216,7 +225,8 @@ pub enum CliPromptRouting { /// baked-vs-deferred routing that gates the deferred-clear recovery path is unit /// testable without a live tmux/socket. Blank prompts route to `None` (the /// caller carries no prompt and clears nothing); anything that fits inline is -/// `Baked`, everything else `Deferred`. +/// `Baked`, everything else `Deferred`. Both variants carry the trimmed text +/// (the downstream [`cli_prompt_file_content`] trim is then a no-op). pub fn route_initial_prompt( initial_prompt: Option, prompt_arg: &CliPromptArg, @@ -228,7 +238,7 @@ pub fn route_initial_prompt( if trimmed.is_empty() { CliPromptRouting::None } else if cli_prompt_fits_inline(prompt_arg, trimmed.len()) { - CliPromptRouting::Baked(prompt) + CliPromptRouting::Baked(trimmed.to_string()) } else { CliPromptRouting::Deferred(trimmed.to_string()) } @@ -847,20 +857,14 @@ pub async fn cli_tmux_session_exists(workspace_id: Uuid) -> bool { return false; } let session_name = cli_tmux_session_name(workspace_id); - tokio::process::Command::new("tmux") - .args([ - "-L", - CLI_TMUX_SOCKET, - "has-session", - "-t", - &format!("={session_name}"), - ]) - .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()) - .status() - .await - .map(|s| s.success()) - .unwrap_or(false) + tmux_ok(&[ + "-L", + CLI_TMUX_SOCKET, + "has-session", + "-t", + &format!("={session_name}"), + ]) + .await } /// Whether CLI mode can actually run claude in a tmux session (vs. degrading @@ -914,17 +918,11 @@ pub async fn cli_pane_agent_running(workspace_id: Uuid) -> Option { .parse::() .ok()?; - // Pane root: normally the bootstrap/fallback shell, but a future exec'd - // agent would show up here directly. - if let Some(comm) = process_comm(pane_pid).await - && !is_shell_command(&comm) - { - return Some(true); - } - - // Direct children of the pane shell (`pgrep -l -P` prints " "; - // a non-zero exit just means no children — the bootstrap is between - // stages or the fallback shell is idle). + // Direct children of the pane shell first — the agent is a direct child + // in every current launch shape, so this is the probe that usually + // answers. (`pgrep -l -P` prints " "; a non-zero exit just + // means no children — the bootstrap is between stages or the fallback + // shell is idle.) let children = tokio::process::Command::new("pgrep") .args(["-l", "-P", &pane_pid.to_string()]) .stderr(std::process::Stdio::null()) @@ -932,15 +930,25 @@ pub async fn cli_pane_agent_running(workspace_id: Uuid) -> Option { .await .ok()?; let listing = String::from_utf8_lossy(&children.stdout); + if listing + .lines() + .filter_map(|line| line.split_whitespace().nth(1)) + .any(|comm| !is_shell_command(comm)) + { + return Some(true); + } + + // Fall back to the pane root: normally the bootstrap/fallback shell, but + // a future exec'd agent would show up here directly. Some( - listing - .lines() - .filter_map(|line| line.split_whitespace().nth(1)) - .any(|comm| !is_shell_command(comm)), + process_comm(pane_pid) + .await + .is_some_and(|comm| !is_shell_command(&comm)), ) } -/// Best-effort process name for a pid (`ps -o comm=`), normalized. +/// Best-effort process name for a pid (`ps -o comm=`), raw (trimmed only — +/// [`is_shell_command`] owns normalization). async fn process_comm(pid: u32) -> Option { let output = tokio::process::Command::new("ps") .args(["-o", "comm=", "-p", &pid.to_string()]) @@ -951,7 +959,7 @@ async fn process_comm(pid: u32) -> Option { if !output.status.success() { return None; } - let comm = normalize_comm(String::from_utf8_lossy(&output.stdout).trim()).to_string(); + let comm = String::from_utf8_lossy(&output.stdout).trim().to_string(); (!comm.is_empty()).then_some(comm) } @@ -966,7 +974,7 @@ fn normalize_comm(comm: &str) -> &str { /// deferred paste: pasting into a shell would execute/mangle the prompt as /// shell input (the bootstrap still starting up, or the missing-binary /// fallback shell), so delivery waits for a non-shell process in the pane. -pub fn is_shell_command(cmd: &str) -> bool { +fn is_shell_command(cmd: &str) -> bool { matches!( normalize_comm(cmd), "sh" | "bash" @@ -1142,20 +1150,25 @@ async fn paste_via_tmux_buffer(workspace_id: Uuid, target: &str, text: &str) -> Err(_) => return false, }; - if let Some(mut stdin) = child.stdin.take() { - if stdin.write_all(text.as_bytes()).await.is_err() { - // Reap the load-buffer child before bailing: dropping the Child does - // not wait() it, so a failed stdin write would otherwise leave a - // lingering/zombie tmux process. Drop stdin first (EOF), then kill - // (which also awaits the exit). - drop(stdin); - let _ = child.kill().await; - return false; - } - // Drop stdin (EOF) so load-buffer completes. - let _ = stdin.shutdown().await; + let Some(mut stdin) = child.stdin.take() else { + // Piped stdin should always be there; a missing handle means the + // buffer can't be loaded, so reap the child and report failure rather + // than pasting an empty buffer. + let _ = child.kill().await; + return false; + }; + if stdin.write_all(text.as_bytes()).await.is_err() { + // Reap the load-buffer child before bailing: dropping the Child does + // not wait() it, so a failed stdin write would otherwise leave a + // lingering/zombie tmux process. Drop stdin first (EOF), then kill + // (which also awaits the exit). drop(stdin); + let _ = child.kill().await; + return false; } + // Drop stdin (EOF) so load-buffer completes. + let _ = stdin.shutdown().await; + drop(stdin); let loaded = child.wait().await.map(|s| s.success()).unwrap_or(false); if !loaded { @@ -1294,8 +1307,10 @@ pub enum PtyError { /// `CreateFailed` so the message reaches the user verbatim — and so we /// never silently drop the prompt by falling through to an empty /// `continue_launch` TUI. The parked DB copy is left untouched for retry. - #[error("{0}")] - PromptStageFailed(String), + /// A unit variant whose display IS the shared recovery notice, so every + /// construction site speaks the same user-facing copy by construction. + #[error("{}", CLI_PROMPT_PARKED_NOTICE)] + PromptStageFailed, #[error("Session not found: {0}")] SessionNotFound(Uuid), #[error("Failed to write to PTY: {0}")] @@ -1363,19 +1378,19 @@ impl PtyService { // CLI mode rides tmux when present; otherwise (and for the // default side terminal) spawn the user's shell directly. - let (tmux_session, tmux_resume_id, tmux_initial_prompt, tmux_spec): ( - Option, + let (tmux_workspace, tmux_resume_id, tmux_initial_prompt, tmux_spec): ( + Option, Option, Option, Option, ) = match &command { PtyCommand::TmuxCli { - session_name, + workspace_id, resume_session_id, initial_prompt, spec, } if tmux_available() => ( - Some(session_name.clone()), + Some(*workspace_id), resume_session_id.clone(), initial_prompt.clone(), Some(spec.clone()), @@ -1386,9 +1401,10 @@ impl PtyService { // Never silently break the persistence promise: if CLI mode was // requested but tmux is absent, say so in the pane itself. if matches!(&command, PtyCommand::TmuxCli { .. }) { - match &tmux_session { - Some(session_name) => tracing::info!( - "CLI terminal attaching tmux session {session_name} in {}", + match tmux_workspace { + Some(workspace_id) => tracing::info!( + "CLI terminal attaching tmux session {} in {}", + cli_tmux_session_name(workspace_id), working_dir.display() ), None => { @@ -1397,9 +1413,10 @@ impl PtyService { } } - let (mut cmd, shell_name) = if let (Some(session_name), Some(spec)) = - (&tmux_session, &tmux_spec) + let (mut cmd, shell_name) = if let (Some(workspace_id), Some(spec)) = + (tmux_workspace, &tmux_spec) { + let session_name = cli_tmux_session_name(workspace_id); // Bring an already-running server in line with our config // (options are server-wide; `-f` below only affects a fresh // server start). @@ -1437,15 +1454,16 @@ impl PtyService { // benign trade vs. the loop. cmd.arg("-A"); cmd.arg("-s"); - cmd.arg(session_name); + cmd.arg(&session_name); cmd.arg("-c"); cmd.arg(&working_dir); // Materialize the initial prompt to a private file so the // bootstrap reads it back rather than carrying it inline // (tmux rejects `new-session` commands past ~16KB). Only when - // an existing conversation won't take precedence and the - // prompt isn't blank; the file self-deletes once consumed. - let resume_active = tmux_resume_id.as_deref().is_some_and(is_uuid); + // an existing conversation won't take precedence (the same + // predicate the bootstrap applies) and the prompt isn't blank; + // the file self-deletes once consumed. + let resume_active = active_resume_id(tmux_resume_id.as_deref()).is_some(); let prompt_file: Option = if resume_active { None } else { @@ -1456,33 +1474,20 @@ impl PtyService { // There is a prompt to deliver: its file MUST be staged. // If the write fails we CANNOT fall through to // `continue_launch` — that yields a healthy-looking but - // empty TUI, and the caller (terminal.rs) would then - // clear the parked DB copy, permanently losing the user's - // only copy of the prompt. Fail the spawn instead so the - // parked prompt survives for the next attach and the user - // sees the recovery notice (the "never destroy the - // prompt" invariant). + // empty TUI that could then be mistaken for delivery. + // Fail the spawn instead so the parked prompt survives + // for the next attach and the user sees the recovery + // notice (the "never destroy the prompt" invariant). Some(content) => { - let wid = workspace_id_from_cli_session_name(session_name).ok_or_else( - || { - tracing::error!( - "CLI session name {session_name} has no workspace id; \ - cannot stage parked prompt" - ); - PtyError::PromptStageFailed( - CLI_PROMPT_PARKED_NOTICE.to_string(), - ) - }, - )?; - Some(write_cli_prompt_file(wid, &content).map_err(|e| { + Some(write_cli_prompt_file(workspace_id, &content).map_err(|e| { tracing::error!( "Failed to write CLI prompt file for {session_name}: \ {e}; leaving prompt parked" ); // Drop any partial file so a torn write can't // leave a stale prompt readable on disk. - remove_cli_prompt_file(wid); - PtyError::PromptStageFailed(CLI_PROMPT_PARKED_NOTICE.to_string()) + remove_cli_prompt_file(workspace_id); + PtyError::PromptStageFailed })?) } None => None, @@ -1921,11 +1926,11 @@ mod tests { // When staging a parked prompt fails (e.g. a full/read-only FS), the // spawn is aborted with PromptStageFailed rather than silently dropping // the prompt. The error text must reach the user verbatim (terminal.rs - // sends `e.to_string()` to the pane), so it carries exactly the same - // recovery copy as the "session never came up" branch — telling the user - // the prompt is saved for the next attach. Guards the user-facing half - // of the "never destroy the prompt" invariant. - let err = PtyError::PromptStageFailed(CLI_PROMPT_PARKED_NOTICE.to_string()); + // sends `e.to_string()` to the pane), and the unit variant makes it + // structurally impossible to construct with different copy than the + // "session never came up" branch — both speak the shared recovery + // notice telling the user the prompt is saved for the next attach. + let err = PtyError::PromptStageFailed; assert_eq!(err.to_string(), CLI_PROMPT_PARKED_NOTICE); assert!(err.to_string().contains("your prompt is saved")); } @@ -1962,11 +1967,11 @@ mod tests { CliPromptRouting::None ); - // Small prompt that fits inline -> Baked, carrying the ORIGINAL - // (untrimmed) text; the bootstrap re-trims + dash-guards downstream. + // Small prompt that fits inline -> Baked, trimmed (the downstream + // file-content trim is then a no-op; the dash guard still applies). assert_eq!( route_initial_prompt(Some(" hi there ".to_string()), &CliPromptArg::Positional), - CliPromptRouting::Baked(" hi there ".to_string()) + CliPromptRouting::Baked("hi there".to_string()) ); // Oversized Positional prompt -> Deferred (paste path), carrying the diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 63521bd50a..7341ce7a35 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -23,8 +23,7 @@ use executors::{ use local_deployment::pty::{ CLI_PROMPT_PARKED_NOTICE, CliPromptDelivery, CliPromptRouting, PtyCommand, cli_pane_agent_running, cli_prompt_file_exists, cli_tmux_available, cli_tmux_session_exists, - cli_tmux_session_name, kill_cli_tmux_session, remove_cli_prompt_file, route_initial_prompt, - send_cli_keys, + kill_cli_tmux_session, remove_cli_prompt_file, route_initial_prompt, send_cli_keys, }; use serde::{Deserialize, Serialize}; use sqlx::SqlitePool; @@ -186,19 +185,9 @@ async fn terminal_ws( )); } - // Session whose parked CLI prompt should be cleared once the tmux session - // is confirmed created (CLI-first first attach only). Set inside the Cli - // arm; cleared post-spawn in handle_terminal_ws. - let mut prompt_session_to_clear: Option = None; - // A large (or otherwise non-inlineable) initial prompt that must be - // delivered by paste AFTER the pane is up, rather than baked into the launch - // command. Set inside the Cli arm; delivered in handle_terminal_ws. - let mut deferred_prompt: Option = None; - // Exclusive in-process claim on this workspace's prompt delivery — held - // whenever this attach carries the prompt, released once the parked copy is - // cleared or deliberately left parked. Racing first-attaches that lose the - // claim attach without the prompt instead of double-staging/double-pasting. - let mut delivery_claim: Option = None; + // Set inside the Cli arm when this attach carries the workspace's parked + // prompt; consumed by handle_terminal_ws to confirm delivery and clear it. + let mut prompt_delivery: Option = None; let (working_dir, command) = match query.mode { TerminalMode::Cli => { @@ -268,17 +257,12 @@ async fn terminal_ws( None => None, }; - // CLI-first creation parks the workspace's initial prompt on the - // session; the tmux bootstrap runs claude with it directly. We - // only PEEK it here (read, don't clear) and only on the genuine - // FIRST attach (no tmux session yet) — reattaches and post-death - // reconnects must not replay it. The clear is deferred until the - // tmux session is confirmed created (see `prompt_session_to_clear` - // below), so a failure between WS upgrade and PTY spawn can't - // destroy the prompt, and two racing first-attaches that both peek - // the same prompt are safe (whichever wins `new-session` carries - // it; the loser's `-A` reattach ignores its bootstrap). An - // existing resumable conversation always wins over a parked prompt. + // A parked prompt (CLI-first creation, or a re-parked loop wake-up) + // is only ever PEEKED here (read, don't clear); the clear happens + // after delivery is CONFIRMED (see handle_terminal_ws), so no + // failure between WS upgrade and agent hand-off can destroy the + // user's only copy. Racing attaches are serialized by the + // CliPromptDelivery claim — the loser attaches without the prompt. // // Gate on tmux availability: with tmux down, CLI mode degrades to // an ephemeral shell that can't run claude, so the bootstrap would @@ -286,40 +270,26 @@ async fn terminal_ws( // Since availability is process-cached, `true` here means // `create_session` also takes the tmux branch, so a successful // spawn really did carry the prompt into a tmux session. - let initial_prompt = match &session { - Some(s) - if resume_session_id.is_none() - && cli_tmux_available() - && !cli_tmux_session_exists(query.workspace_id).await => - { - // Two attaches can both pass the no-session-yet gate before - // either spawns; only the claim holder carries the prompt - // (the loser's `-A` attach ignores its bootstrap anyway), - // so the prompt file can't be re-truncated mid-`cat` and an - // oversized prompt can't be pasted twice. + let carried: Option<(String, CliPromptDelivery, Uuid)> = match &session { + Some(s) if cli_tmux_available() => { match CliPromptDelivery::try_claim(query.workspace_id) { - Some(claim) => { - match Session::peek_pending_cli_prompt(pool, s.id).await { - Ok(Some(prompt)) => { - delivery_claim = Some(claim); - Some(prompt) - } - // Nothing parked: release the claim (drop). - Ok(None) => None, - Err(e) => { - // The prompt is not lost — it stays parked - // and the next attach re-peeks — but a - // transient DB error here delays delivery, - // so make it observable. - tracing::warn!( - "Failed to read pending CLI prompt for session {}: {}", - s.id, - e - ); - None - } + Some(claim) => match Session::peek_pending_cli_prompt(pool, s.id).await { + Ok(Some(prompt)) => Some((prompt, claim, s.id)), + // Nothing parked: release the claim (drop). + Ok(None) => None, + Err(e) => { + // The prompt is not lost — it stays parked and + // the next attach re-peeks — but a transient DB + // error here delays delivery, so make it + // observable. + tracing::warn!( + "Failed to read pending CLI prompt for session {}: {}", + s.id, + e + ); + None } - } + }, None => { tracing::debug!( "CLI prompt delivery for workspace {} already in flight; \ @@ -337,34 +307,63 @@ async fn terminal_ws( let (model_id, reasoning_id) = resolve_cli_model_effort(pool, session.as_ref()).await; let spec = resolve_cli_launch_spec(session.as_ref(), model_id, reasoning_id, &dir); - // Small prompts ride the bootstrap's temp-file transport (baked into - // the launch). Prompts too large to pass as one argv entry — or - // agents with no launch-time prompt arg — are delivered by paste - // after the pane is confirmed up (see handle_terminal_ws), so they're - // never truncated and never silently lost. Either way the parked - // prompt is cleared only after delivery is confirmed. - let baked_prompt = match route_initial_prompt(initial_prompt, &spec.prompt_arg) { - CliPromptRouting::None => None, - CliPromptRouting::Baked(prompt) => Some(prompt), - CliPromptRouting::Deferred(prompt) => { - deferred_prompt = Some(prompt); - None + // How the parked prompt travels: + // - Genuine first attach (no tmux session yet, nothing to resume): + // small prompts ride the bootstrap's temp-file transport (baked + // into the launch); prompts too large for one argv entry are + // pasted after the agent owns the pane. + // - Otherwise (the session already exists — e.g. an earlier + // delivery went unconfirmed, or a loop wake-up was re-parked — + // or an existing conversation is being resumed, which always + // wins the launch itself): deliver by paste into the running + // agent, as a follow-up. Without this branch a parked prompt + // behind a live session or a resume would be stranded forever. + // Either way the parked prompt is cleared only after delivery is + // confirmed. + let mut baked_prompt = None; + if let Some((peeked, claim, clear_session_id)) = carried { + let fresh_launch = resume_session_id.is_none() + && !cli_tmux_session_exists(query.workspace_id).await; + let routed = if fresh_launch { + route_initial_prompt(Some(peeked.clone()), &spec.prompt_arg) + } else { + let trimmed = peeked.trim(); + if trimmed.is_empty() { + CliPromptRouting::None + } else { + CliPromptRouting::Deferred(trimmed.to_string()) + } + }; + match routed { + // Blank-after-trim: nothing will be delivered; the claim + // drops here rather than being held across the attach. + CliPromptRouting::None => {} + CliPromptRouting::Baked(prompt) => { + baked_prompt = Some(prompt); + prompt_delivery = Some(PromptDelivery { + workspace_id: query.workspace_id, + clear_session_id, + peeked, + deferred: None, + claim, + }); + } + CliPromptRouting::Deferred(prompt) => { + prompt_delivery = Some(PromptDelivery { + workspace_id: query.workspace_id, + clear_session_id, + peeked, + deferred: Some(prompt), + claim, + }); + } } - }; - // Remember which session's prompt to clear once delivery is - // confirmed (only when we actually carried a prompt). - if baked_prompt.is_some() || deferred_prompt.is_some() { - prompt_session_to_clear = session.as_ref().map(|s| s.id); - } else { - // Blank-after-trim prompt: nothing will be delivered, so don't - // hold the delivery claim across the attach. - delivery_claim = None; } ( dir, PtyCommand::TmuxCli { - session_name: cli_tmux_session_name(query.workspace_id), + workspace_id: query.workspace_id, resume_session_id, initial_prompt: baked_prompt, spec, @@ -405,15 +404,32 @@ async fn terminal_ws( query.cols, query.rows, command, - query.workspace_id, - prompt_session_to_clear, - deferred_prompt, - delivery_claim, + prompt_delivery, ) })) } -#[allow(clippy::too_many_arguments)] +/// Everything a prompt-carrying CLI attach needs to confirm delivery and clear +/// the parked prompt. Built only when this attach actually carries the prompt +/// (it holds the claim), so "claim without prompt" or "deferred text without a +/// session to clear" are unrepresentable. +struct PromptDelivery { + workspace_id: Uuid, + /// Session whose parked prompt to clear once delivery is confirmed. + clear_session_id: Uuid, + /// The exact parked value that was peeked — the clear is a compare-and-swap + /// against it, so a NEWER prompt parked mid-confirmation (e.g. a loop + /// wake-up re-parked while this delivery was in flight) is never destroyed + /// by this delivery's clear. + peeked: String, + /// Prompt to paste once the agent owns the pane (`None` = baked into the + /// launch's temp-file transport instead). + deferred: Option, + /// Exclusive in-process claim; held until the parked copy is cleared or + /// deliberately left parked, releasing on drop. + claim: CliPromptDelivery, +} + async fn handle_terminal_ws( mut socket: MaybeSignedWebSocket, deployment: DeploymentImpl, @@ -421,10 +437,7 @@ async fn handle_terminal_ws( cols: u16, rows: u16, command: PtyCommand, - workspace_id: Uuid, - prompt_session_to_clear: Option, - deferred_prompt: Option, - delivery_claim: Option, + prompt_delivery: Option, ) { let (session_id, mut output_rx) = match deployment .pty() @@ -437,8 +450,8 @@ async fn handle_terminal_ws( // A prompt file staged before the spawn failure must not outlive // it (the DB copy stays parked for the next attach; the claim is // released on return). - if prompt_session_to_clear.is_some() { - remove_cli_prompt_file(workspace_id); + if let Some(delivery) = &prompt_delivery { + remove_cli_prompt_file(delivery.workspace_id); } let _ = send_error(&mut socket, &e.to_string()).await; return; @@ -451,7 +464,8 @@ async fn handle_terminal_ws( // actually exists before doing anything with the parked prompt. A session // that never comes up leaves the prompt saved for the next attach and tells // the user so, instead of silently destroying it. - if let Some(clear_session_id) = prompt_session_to_clear { + if let Some(delivery) = prompt_delivery { + let workspace_id = delivery.workspace_id; if wait_for_cli_session(workspace_id).await { // Confirm delivery and clear the parked prompt in the background so // the pane's output starts streaming immediately — the confirmation @@ -460,23 +474,38 @@ async fn handle_terminal_ws( tokio::spawn(async move { // Hold the exclusive delivery claim for the whole // confirm-then-clear window. - let _claim = delivery_claim; - let delivered = match deferred_prompt { - Some(text) => deliver_deferred_prompt(workspace_id, &text).await, + let _claim = delivery.claim; + let delivered = match &delivery.deferred { + Some(text) => deliver_deferred_prompt(workspace_id, text).await, None => confirm_baked_prompt_consumed(workspace_id).await, }; if delivered { - if let Err(e) = Session::clear_pending_cli_prompt(&pool, clear_session_id).await + match Session::clear_pending_cli_prompt( + &pool, + delivery.clear_session_id, + &delivery.peeked, + ) + .await { - tracing::warn!( + // Superseded: a newer prompt was parked while this one + // was being confirmed; it stays parked for its own + // delivery on the next attach. + Ok(false) => tracing::info!( + "Parked CLI prompt for session {} changed during delivery; \ + left for the next attach", + delivery.clear_session_id + ), + Ok(true) => {} + Err(e) => tracing::warn!( "Failed to clear delivered CLI prompt for session {}: {}", - clear_session_id, + delivery.clear_session_id, e - ); + ), } } else { - // Leave the prompt parked; a later fresh session delivers - // it (delivery is only ever confirmed, never assumed). + // Leave the prompt parked; the next attach retries (paste + // into the live pane, or a fresh launch after the session + // dies) — delivery is only ever confirmed, never assumed. tracing::warn!( "CLI prompt delivery for workspace {} unconfirmed; left parked", workspace_id @@ -578,16 +607,18 @@ async fn wait_for_cli_session(workspace_id: Uuid) -> bool { false } -/// Confirm the pane's bootstrap actually consumed the staged (baked) prompt -/// file. The bootstrap `rm`s the file the moment it hands the prompt to the -/// agent, so "file gone" is a real delivery acknowledgement — while a launch -/// that never consumed it (agent binary missing → the bootstrap's `command -v` -/// guard skips the launch arm and drops to the fallback shell) leaves the file -/// behind, and the parked DB prompt must then survive for a later fresh -/// session instead of being cleared into the void. +/// Confirm a baked prompt was actually handed to the agent: the staged file is +/// gone (the bootstrap `rm`s it at hand-off — see [`cli_prompt_file_exists`]) +/// AND a non-shell process owns the pane. The second condition catches the +/// consumed-but-never-executed window (`command -v` passed but the agent's +/// exec failed: bad shebang, loader error, permissions) where file-gone alone +/// would clear a prompt no agent ever received. An unconfirmed launch leaves +/// the parked DB prompt for the next attach's paste/fresh-launch retry. async fn confirm_baked_prompt_consumed(workspace_id: Uuid) -> bool { for _ in 0..20 { - if !cli_prompt_file_exists(workspace_id) { + if !cli_prompt_file_exists(workspace_id) + && cli_pane_agent_running(workspace_id).await == Some(true) + { return true; } tokio::time::sleep(std::time::Duration::from_millis(200)).await; From 980eba4aecaf78462d5c100655de4c002e2dc0b0 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 21:37:44 +0000 Subject: [PATCH 06/14] =?UTF-8?q?fix(cli-mode):=20close=20xhigh-review=20d?= =?UTF-8?q?elivery=20races=20=E2=80=94=20expected-agent=20gate,=20guarded?= =?UTF-8?q?=20re-park,=20workspace=20peek?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recall-mode review (5 correctness finders + conventions) on the delivery state machine: - Deferred paste gates on THE agent we launched (tmux #{pane_pid} tree comm == spec.program, 15-byte comm truncation handled) instead of any non-shell process — a user running vim/npm inside the missing-binary fallback shell can no longer receive (and effectively destroy) a prompt meant for the agent. Re-verified right before the paste (the agent can die inside the grace window and drop the pane to a shell that would EXECUTE the prompt) and after it (an agent that exits immediately discarded the paste with its tty; delivery is not confirmed). - Fresh deferred launches start a bare TUI instead of --continue||fresh: the doomed --continue first leg of a brand-new workspace lived just long enough to pass the readiness gate, swallow the paste, exit, and let the CAS clear destroy the prompt. - Multi-line texts always ride the bracketed-paste buffer transport; send-keys -l types newlines as Enter, submitting at the first line while reporting success. Enter is retried once and a text-delivered/Enter-failed send now counts as delivered (re-pasting would double the text). - Loop wake-up re-parks park only into an EMPTY slot (guarded UPDATE): continuation boilerplate can never overwrite a parked-but-undelivered user prompt. Wake-up sends take the CliPromptDelivery claim so they can't interleave paste+Enter pairs with an in-flight delivery; a busy claim defers the wake-up to the next tick. - The parked-prompt peek is workspace-scoped (creation parks on the CLI-first session, wake-ups on the latest, the attach may resolve a third) so a prompt parked on a sibling session row is never stranded. - kill_cli_tmux_session kills before removing the prompt file (a first rm could yank the file mid-bootstrap and let file-gone+agent-up confirm a delivery that never happened); the session-never-came-up branch no longer kills a late-appearing session (it may be running real work; a parked prompt behind a live session is deliverable now); blank parked prompts are CAS-cleared instead of re-probed forever; the baked confirm window widened to 30x250ms and drops the never-consumed file on timeout; the claim registry recovers from mutex poisoning instead of silently disabling all deliveries; the embedded tmux conf pins default-shell /bin/sh so the POSIX bootstrap survives fish/csh login shells (applied to running servers too). Tests: in-memory SQLite coverage for the CAS clear (match/supersede/no-op), if-empty park, and workspace-scoped peek; unit tests for the fresh-deferred bootstrap, follow-up routing, paste-transport predicate, and expected-agent comm matching. --- ...5b162015a01f5dea425881cb99dfcaa85ecc2.json | 12 + ...d3a208ea541046b11130dc851c262353f756a.json | 26 ++ crates/db/src/models/session.rs | 46 +++ .../local-deployment/src/loop_supervisor.rs | 33 +- crates/local-deployment/src/pty.rs | 312 +++++++++++------ crates/server/src/routes/terminal.rs | 320 +++++++++++++++--- 6 files changed, 594 insertions(+), 155 deletions(-) create mode 100644 crates/db/.sqlx/query-dce20912fb7e4bec31853c398815b162015a01f5dea425881cb99dfcaa85ecc2.json create mode 100644 crates/db/.sqlx/query-e0697566b6b789daa74f54a31bdd3a208ea541046b11130dc851c262353f756a.json diff --git a/crates/db/.sqlx/query-dce20912fb7e4bec31853c398815b162015a01f5dea425881cb99dfcaa85ecc2.json b/crates/db/.sqlx/query-dce20912fb7e4bec31853c398815b162015a01f5dea425881cb99dfcaa85ecc2.json new file mode 100644 index 0000000000..a8ea22129a --- /dev/null +++ b/crates/db/.sqlx/query-dce20912fb7e4bec31853c398815b162015a01f5dea425881cb99dfcaa85ecc2.json @@ -0,0 +1,12 @@ +{ + "db_name": "SQLite", + "query": "UPDATE sessions\n SET pending_cli_prompt = $1\n WHERE id = $2 AND pending_cli_prompt IS NULL", + "describe": { + "columns": [], + "parameters": { + "Right": 2 + }, + "nullable": [] + }, + "hash": "dce20912fb7e4bec31853c398815b162015a01f5dea425881cb99dfcaa85ecc2" +} diff --git a/crates/db/.sqlx/query-e0697566b6b789daa74f54a31bdd3a208ea541046b11130dc851c262353f756a.json b/crates/db/.sqlx/query-e0697566b6b789daa74f54a31bdd3a208ea541046b11130dc851c262353f756a.json new file mode 100644 index 0000000000..e78dda053c --- /dev/null +++ b/crates/db/.sqlx/query-e0697566b6b789daa74f54a31bdd3a208ea541046b11130dc851c262353f756a.json @@ -0,0 +1,26 @@ +{ + "db_name": "SQLite", + "query": "SELECT id AS \"id!: Uuid\", pending_cli_prompt AS \"prompt!: String\"\n FROM sessions\n WHERE workspace_id = $1 AND pending_cli_prompt IS NOT NULL\n ORDER BY created_at DESC\n LIMIT 1", + "describe": { + "columns": [ + { + "name": "id!: Uuid", + "ordinal": 0, + "type_info": "Blob" + }, + { + "name": "prompt!: String", + "ordinal": 1, + "type_info": "Text" + } + ], + "parameters": { + "Right": 1 + }, + "nullable": [ + true, + true + ] + }, + "hash": "e0697566b6b789daa74f54a31bdd3a208ea541046b11130dc851c262353f756a" +} diff --git a/crates/db/src/models/session.rs b/crates/db/src/models/session.rs index 16d71e963a..94431ff2cc 100644 --- a/crates/db/src/models/session.rs +++ b/crates/db/src/models/session.rs @@ -210,6 +210,52 @@ impl Session { Ok(()) } + /// Park `prompt` ONLY if nothing is parked yet. Loop wake-up re-parks use + /// this so continuation boilerplate can never overwrite a parked-but- + /// undelivered user prompt — the "never destroy the prompt" invariant has + /// to hold on the write side too, not just the CAS-guarded clear. Returns + /// whether the park happened; a skipped park is fine (the parked prompt + /// is delivered first and the loop re-detects its limit banner). + pub async fn set_pending_cli_prompt_if_empty( + pool: &SqlitePool, + id: Uuid, + prompt: &str, + ) -> Result { + let result = sqlx::query!( + r#"UPDATE sessions + SET pending_cli_prompt = $1 + WHERE id = $2 AND pending_cli_prompt IS NULL"#, + prompt, + id + ) + .execute(pool) + .await?; + Ok(result.rows_affected() > 0) + } + + /// The workspace's parked CLI prompt, wherever it lives: creation parks it + /// on the CLI-first session, loop wake-ups re-park on the LATEST session, + /// and an attach may resolve a third (frontend-selected) session — so the + /// peek must be workspace-scoped or a prompt parked on a sibling session + /// row would be stranded forever. Returns the owning session's id (for + /// the eventual CAS clear) and the prompt. + pub async fn peek_pending_cli_prompt_for_workspace( + pool: &SqlitePool, + workspace_id: Uuid, + ) -> Result, sqlx::Error> { + let row = sqlx::query!( + r#"SELECT id AS "id!: Uuid", pending_cli_prompt AS "prompt!: String" + FROM sessions + WHERE workspace_id = $1 AND pending_cli_prompt IS NOT NULL + ORDER BY created_at DESC + LIMIT 1"#, + workspace_id + ) + .fetch_optional(pool) + .await?; + Ok(row.map(|r| (r.id, r.prompt))) + } + /// Read the parked CLI prompt WITHOUT clearing it. The clear is deferred /// to [`clear_pending_cli_prompt`] until delivery is CONFIRMED — the /// launch bootstrap consumed the staged prompt file with the agent up, or diff --git a/crates/local-deployment/src/loop_supervisor.rs b/crates/local-deployment/src/loop_supervisor.rs index c36760acf2..b449413f9b 100644 --- a/crates/local-deployment/src/loop_supervisor.rs +++ b/crates/local-deployment/src/loop_supervisor.rs @@ -28,7 +28,9 @@ use db::{ }; use uuid::Uuid; -use crate::pty::{capture_cli_pane, cli_tmux_available, cli_tmux_session_exists, send_cli_keys}; +use crate::pty::{ + CliPromptDelivery, capture_cli_pane, cli_tmux_available, cli_tmux_session_exists, send_cli_keys, +}; /// How often to poll panes for limit banners and check for due wake-ups. const POLL_INTERVAL: Duration = Duration::from_secs(20); @@ -443,6 +445,18 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), } } + // Serialize with any in-flight parked-prompt delivery (terminal + // attach): two concurrent paste+Enter pairs into the same pane would + // interleave into one garbled submission. If a delivery holds the + // claim, leave the wake-up pending — the next tick retries after the + // delivery window closes. + let Some(_claim) = CliPromptDelivery::try_claim(wid) else { + tracing::debug!( + "loop: prompt delivery in flight for workspace {wid}; deferring wake-up" + ); + continue; + }; + if send_cli_keys(wid, &prompt).await { if is_limit { let _ = LoopAutomation::increment_attempts(pool, wid).await; @@ -466,12 +480,23 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), /// Best-effort: park `prompt` on the workspace's latest session so a terminal /// attach delivers it (the attach path pastes a parked prompt into a live /// agent pane, or hands it to the next fresh launch). Shared by both wake-up -/// failure branches so the re-park policy has exactly one owner. +/// failure branches so the re-park policy has exactly one owner. Parks only +/// into an EMPTY slot: continuation boilerplate must never overwrite a +/// parked-but-undelivered user prompt (that prompt delivers first, and the +/// loop re-detects its limit banner afterwards). async fn repark_prompt(pool: &sqlx::SqlitePool, wid: Uuid, prompt: &str) { match Session::find_latest_by_workspace_id(pool, wid).await { Ok(Some(session)) => { - if let Err(e) = Session::set_pending_cli_prompt(pool, session.id, prompt).await { - tracing::warn!("loop: failed to re-park wake-up prompt for workspace {wid}: {e}"); + match Session::set_pending_cli_prompt_if_empty(pool, session.id, prompt).await { + Ok(true) => {} + Ok(false) => tracing::info!( + "loop: workspace {wid} already has a parked prompt; wake-up not re-parked" + ), + Err(e) => { + tracing::warn!( + "loop: failed to re-park wake-up prompt for workspace {wid}: {e}" + ) + } } } Ok(None) => { diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index e7a0b91b9d..220c2d1279 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -44,6 +44,12 @@ pub enum PtyCommand { /// the terminal instead of a headless executor. Ignored when /// `resume_session_id` is set. initial_prompt: Option, + /// A paste delivery will follow this launch (oversized or + /// launch-arg-less prompt). The bootstrap must start a FRESH TUI + /// instead of the `--continue || fresh` fallback: the doomed + /// `--continue` first leg of a brand-new workspace lives just long + /// enough to swallow the paste and exit, silently discarding it. + deferred_prompt_pending: bool, /// How to launch the selected agent's interactive CLI — binary, flags /// (model/effort/sandbox/approval pre-resolved from the session's /// ExecutorConfig), and the resume/prompt/continue forms. Claude is the @@ -77,6 +83,7 @@ fn cli_bootstrap( spec: &CliLaunchSpec, resume_session_id: Option<&str>, prompt_file: Option<&Path>, + deferred_prompt_pending: bool, ) -> String { // The program is a bare binary name from our own code; quote it anyway so // it can never be anything but a single command word. @@ -155,6 +162,12 @@ fn cli_bootstrap( // post-launch keystroke delivery (loop automation / send-keys). CliPromptArg::Unsupported => continue_launch(), } + } else if deferred_prompt_pending { + // A paste delivery follows: start a FRESH TUI. The usual + // `--continue || fresh` fallback would run a doomed `--continue` + // first leg on a brand-new workspace — alive just long enough to + // pass the readiness gate, swallow the pasted prompt, and exit. + base.clone() } else { continue_launch() }; @@ -244,6 +257,20 @@ pub fn route_initial_prompt( } } +/// Route a parked prompt that must arrive as a FOLLOW-UP — the tmux session +/// already exists (an earlier delivery went unconfirmed, or a loop wake-up +/// was re-parked) or a resume launch wins the boot: never baked, always +/// pasted into the running agent. Blank prompts route to `None`. Pure for the +/// same reason as [`route_initial_prompt`]. +pub fn route_followup_prompt(prompt: &str) -> CliPromptRouting { + let trimmed = prompt.trim(); + if trimmed.is_empty() { + CliPromptRouting::None + } else { + CliPromptRouting::Deferred(trimmed.to_string()) + } +} + /// The exact bytes to write to a workspace's CLI prompt file for `prompt_arg`, /// or `None` when the (trimmed) prompt is blank — mirroring the old in-command /// quoting semantics so small prompts behave identically: the leading-dash @@ -358,21 +385,29 @@ impl CliPromptDelivery { /// this process already holds it. pub fn try_claim(workspace_id: Uuid) -> Option { let set = CLI_PROMPT_DELIVERIES.get_or_init(Default::default); + // Recover from poisoning rather than propagating it: the set is a + // plain HashSet with no invariants a panicked holder could have + // broken, and treating poison as "already claimed" would silently + // disable prompt delivery for EVERY workspace until restart. + // // Release the registry guard BEFORE constructing the claim: an eagerly // built `Self` on the not-inserted path (`then_some`) would be dropped // while the guard is still alive, and `Drop` re-locks the same // (non-reentrant) mutex — a self-deadlock. - let inserted = set.lock().ok()?.insert(workspace_id); + let inserted = set + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert(workspace_id); inserted.then(|| Self(workspace_id)) } } impl Drop for CliPromptDelivery { fn drop(&mut self) { - if let Some(set) = CLI_PROMPT_DELIVERIES.get() - && let Ok(mut set) = set.lock() - { - set.remove(&self.0); + if let Some(set) = CLI_PROMPT_DELIVERIES.get() { + set.lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .remove(&self.0); } } } @@ -766,12 +801,18 @@ fn seed_codex_update_dismissal() -> std::io::Result<()> { /// no keystroke. /// - right-click is unbound from tmux's context menu so the web terminal can /// use it for paste. +/// - `default-shell /bin/sh`: window command strings (our launch bootstrap) +/// are run via `default-shell -c`, and the bootstrap is POSIX sh +/// (`vk_p="$(cat …)"`, `{ …; } | …`) — a fish/csh login shell would fail on +/// it outright. The pane still ends in the USER'S shell: the bootstrap's +/// final `exec "${SHELL:-/bin/sh}"` honors `$SHELL`. const CLI_TMUX_CONF: &str = "\ # BetterCoding embedded terminal tmux server (socket: vibe-kanban). # Written by the backend before each CLI terminal attach - edits are overwritten. set -g mouse on set -s set-clipboard on set -as terminal-features ',xterm*:clipboard' +set -g default-shell /bin/sh unbind-key -n MouseDown3Pane "; @@ -801,6 +842,12 @@ fn ensure_cli_tmux_server_options() { .output() }; + // default-shell governs how each NEW window command string (our POSIX-sh + // launch bootstrap) is parsed; apply unconditionally (idempotent, no-op + // without a running server) so servers started before this option joined + // the conf don't hand the bootstrap to a fish/csh login shell. + let _ = tmux(&["set-option", "-g", "default-shell", "/bin/sh"]); + let Ok(probe) = tmux(&["show-options", "-s", "set-clipboard"]) else { return; }; @@ -877,19 +924,21 @@ pub fn cli_tmux_available() -> bool { tmux_available() } -/// Whether the AGENT (a non-shell process) currently owns a workspace's CLI -/// pane. `None` when the pane itself can't be read (session gone). +/// Whether THE EXPECTED AGENT (`program`, the spec's binary name) currently +/// owns a workspace's CLI pane. `None` when the pane itself can't be read +/// (session gone). /// /// tmux runs our bootstrap string via `default-shell -c`, and every launch /// stage shares that shell's process group — so `#{pane_current_command}` /// reports the outer shell for the pane's whole life (verified empirically) /// and can't distinguish "agent running" from "fallback shell". Instead this /// inspects the pane's process tree: the pane root (`#{pane_pid}`) or one of -/// its direct children being a non-shell process means the agent is up. The -/// bootstrap keeps the agent a direct child of the pane shell (it is not -/// `exec`ed), and the missing-binary fallback `exec`s an interactive shell -/// with no children — both shapes verified on a scratch socket. -pub async fn cli_pane_agent_running(workspace_id: Uuid) -> Option { +/// its direct children must BE the expected agent. Matching the exact program +/// name (shebang exec sets the process comm to the script basename — verified +/// on a scratch socket) rather than "any non-shell" means a user running +/// vim/npm inside the missing-binary fallback shell can never satisfy the +/// paste gate and receive a prompt meant for the agent. +pub async fn cli_pane_agent_running(workspace_id: Uuid, program: &str) -> Option { if !tmux_available() { return None; } @@ -933,7 +982,7 @@ pub async fn cli_pane_agent_running(workspace_id: Uuid) -> Option { if listing .lines() .filter_map(|line| line.split_whitespace().nth(1)) - .any(|comm| !is_shell_command(comm)) + .any(|comm| comm_matches_program(comm, program)) { return Some(true); } @@ -943,10 +992,21 @@ pub async fn cli_pane_agent_running(workspace_id: Uuid) -> Option { Some( process_comm(pane_pid) .await - .is_some_and(|comm| !is_shell_command(&comm)), + .is_some_and(|comm| comm_matches_program(&comm, program)), ) } +/// Whether a `ps`/`pgrep` process name is the expected agent binary. The +/// kernel truncates comm to 15 bytes (`TASK_COMM_LEN` - 1), so long program +/// names match on their truncated prefix; comparison happens on the +/// normalized basename so `/usr/local/bin/claude` and `claude` agree. +fn comm_matches_program(comm: &str, program: &str) -> bool { + let comm = normalize_comm(comm); + let program = normalize_comm(program); + comm == program + || (program.len() > 15 && program.get(..15).is_some_and(|prefix| comm == prefix)) +} + /// Best-effort process name for a pid (`ps -o comm=`), raw (trimmed only — /// [`is_shell_command`] owns normalization). async fn process_comm(pid: u32) -> Option { @@ -963,35 +1023,13 @@ async fn process_comm(pid: u32) -> Option { (!comm.is_empty()).then_some(comm) } -/// Normalize a `ps`/`pgrep` process name for shell detection: login shells -/// report as `-zsh`, and macOS `ps -o comm=` can report a full path. +/// Normalize a `ps`/`pgrep` process name for comparison: login shells report +/// as `-zsh`, and macOS `ps -o comm=` can report a full path. fn normalize_comm(comm: &str) -> &str { let comm = comm.trim().trim_start_matches('-'); comm.rsplit('/').next().unwrap_or(comm) } -/// Whether a process name is a shell — i.e. NOT the agent. Used to gate the -/// deferred paste: pasting into a shell would execute/mangle the prompt as -/// shell input (the bootstrap still starting up, or the missing-binary -/// fallback shell), so delivery waits for a non-shell process in the pane. -fn is_shell_command(cmd: &str) -> bool { - matches!( - normalize_comm(cmd), - "sh" | "bash" - | "zsh" - | "dash" - | "ash" - | "ksh" - | "mksh" - | "csh" - | "tcsh" - | "fish" - | "nu" - | "busybox" - | "login" - ) -} - /// Inverse of [`cli_tmux_session_name`]: recover the workspace id from one of /// our tmux session names. Returns `None` for anything outside the `vk_` /// namespace (e.g. a user-created session on the same socket). @@ -1007,30 +1045,33 @@ pub(crate) fn workspace_id_from_cli_session_name(name: &str) -> Option { /// cleanup so sessions don't outlive their worktree). `=` forces exact-name /// matching — tmux `-t` is otherwise a prefix match. pub async fn kill_cli_tmux_session(workspace_id: Uuid) { - // Drop any transient prompt file alongside the session (covers the + if tmux_available() { + let session_name = cli_tmux_session_name(workspace_id); + match tokio::process::Command::new("tmux") + .args([ + "-L", + CLI_TMUX_SOCKET, + "kill-session", + "-t", + &format!("={session_name}"), + ]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .await + { + // Non-success simply means no such session — the common case. + Ok(_) => {} + Err(e) => tracing::debug!("Failed to run tmux kill-session for {session_name}: {e}"), + } + } + // Drop any transient prompt file AFTER the session is dead (covers the // never-attached case where the bootstrap never ran to self-delete it). + // Order matters: removing the file first could yank it from under a + // bootstrap that is between `command -v` and `cat` — launching the agent + // with an empty prompt while a concurrent delivery confirmation reads + // "file gone + agent up" as delivered and clears the parked DB copy. remove_cli_prompt_file(workspace_id); - if !tmux_available() { - return; - } - let session_name = cli_tmux_session_name(workspace_id); - match tokio::process::Command::new("tmux") - .args([ - "-L", - CLI_TMUX_SOCKET, - "kill-session", - "-t", - &format!("={session_name}"), - ]) - .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()) - .status() - .await - { - // Non-success simply means no such session — the common case. - Ok(_) => {} - Err(e) => tracing::debug!("Failed to run tmux kill-session for {session_name}: {e}"), - } } /// Capture the visible content of a workspace's CLI tmux pane (best-effort). @@ -1084,13 +1125,23 @@ async fn tmux_ok(args: &[&str]) -> bool { .unwrap_or(false) } +/// Whether `text` must ride the buffer/bracketed-paste transport instead of +/// plain `send-keys -l`: oversized texts hit tmux's ~16KB client-command +/// ceiling, and MULTI-LINE texts sent as literal keystrokes would submit at +/// the first newline (each `\n` acts as Enter to the TUI) — mangling the +/// message while still reporting success. Single-line small texts keep the +/// live-verified `send-keys -l` path. +fn needs_paste_transport(text: &str) -> bool { + text.len() >= SEND_KEYS_PASTE_THRESHOLD || text.contains('\n') +} + /// Type `text` into a workspace's CLI tmux pane and submit it (Enter), as if the /// user typed it. This is the only way to re-prompt a LIVE, detached agent: the /// parked `pending_cli_prompt` path only fires when a fresh tmux session is -/// created, so an already-running pane needs keystroke injection. Small texts go -/// via `send-keys -l` (literal, so never interpreted as tmux key names); larger -/// texts are staged through a namespaced tmux buffer and bracketed-pasted (no -/// argv-length ceiling; embedded newlines don't submit early). Enter is a +/// created, so an already-running pane needs keystroke injection. Small +/// single-line texts go via `send-keys -l` (literal, so never interpreted as +/// tmux key names); larger or multi-line texts are staged through a namespaced +/// tmux buffer and bracketed-pasted ([`needs_paste_transport`]). Enter is a /// separate call so it submits rather than being typed verbatim. Best-effort. pub async fn send_cli_keys(workspace_id: Uuid, text: &str) -> bool { if !tmux_available() { @@ -1101,7 +1152,9 @@ pub async fn send_cli_keys(workspace_id: Uuid, text: &str) -> bool { // full-hex names. let target = cli_tmux_session_name(workspace_id); - let delivered = if text.len() < SEND_KEYS_PASTE_THRESHOLD { + let delivered = if needs_paste_transport(text) { + paste_via_tmux_buffer(workspace_id, &target, text).await + } else { tmux_ok(&[ "-L", CLI_TMUX_SOCKET, @@ -1112,14 +1165,26 @@ pub async fn send_cli_keys(workspace_id: Uuid, text: &str) -> bool { text, ]) .await - } else { - paste_via_tmux_buffer(workspace_id, &target, text).await }; if !delivered { return false; } - tmux_ok(&["-L", CLI_TMUX_SOCKET, "send-keys", "-t", &target, "Enter"]).await + // The text is already IN the pane; failing the whole send over a flaky + // Enter would make the caller re-deliver the text on top of the residue + // (a doubled prompt). Retry the Enter once, then accept: delivered (the + // user can press Enter themselves), submission best-effort. + for _ in 0..2 { + if tmux_ok(&["-L", CLI_TMUX_SOCKET, "send-keys", "-t", &target, "Enter"]).await { + return true; + } + tokio::time::sleep(std::time::Duration::from_millis(250)).await; + } + tracing::warn!( + "CLI send to workspace {workspace_id}: text delivered but Enter failed; \ + left unsubmitted in the pane" + ); + true } /// Stage `text` into a per-workspace tmux buffer via `load-buffer -` (text on @@ -1378,24 +1443,27 @@ impl PtyService { // CLI mode rides tmux when present; otherwise (and for the // default side terminal) spawn the user's shell directly. - let (tmux_workspace, tmux_resume_id, tmux_initial_prompt, tmux_spec): ( + let (tmux_workspace, tmux_resume_id, tmux_initial_prompt, tmux_deferred, tmux_spec): ( Option, Option, Option, + bool, Option, ) = match &command { PtyCommand::TmuxCli { workspace_id, resume_session_id, initial_prompt, + deferred_prompt_pending, spec, } if tmux_available() => ( Some(*workspace_id), resume_session_id.clone(), initial_prompt.clone(), + *deferred_prompt_pending, Some(spec.clone()), ), - _ => (None, None, None, None), + _ => (None, None, None, false, None), }; // Never silently break the persistence promise: if CLI mode was @@ -1497,6 +1565,7 @@ impl PtyService { spec, tmux_resume_id.as_deref(), prompt_file.as_deref(), + tmux_deferred, )); cmd.cwd(&working_dir); // No shell-specific prompt configuration for the tmux client. @@ -1747,7 +1816,7 @@ mod tests { #[test] fn cli_bootstrap_runs_program_then_drops_to_shell() { - let b = cli_bootstrap(&claude_spec(&[]), None, None); + let b = cli_bootstrap(&claude_spec(&[]), None, None, false); assert!(b.contains("command -v 'claude'")); assert!( b.ends_with(r#"exec "${SHELL:-/bin/sh}""#), @@ -1759,7 +1828,7 @@ mod tests { fn cli_bootstrap_warns_with_install_hint_when_agent_missing() { // A not-installed agent must explain itself instead of silently dropping // to a bare shell. - let b = cli_bootstrap(&claude_spec(&[]), None, None); + let b = cli_bootstrap(&claude_spec(&[]), None, None, false); assert!(b.contains("if command -v 'claude'")); assert!(b.contains("is not installed or not on PATH")); assert!( @@ -1779,6 +1848,7 @@ mod tests { &claude_spec(&[]), Some(id), Some(Path::new("/tmp/vk/prompt.txt")), + false, ); assert!(b.contains(&format!("--resume {id}"))); // The prompt file is ignored entirely when resuming. @@ -1786,7 +1856,7 @@ mod tests { assert!(!b.contains("vk_p=")); // Non-UUID (injection attempt) is rejected and never interpolated. let evil = "x; rm -rf ~"; - let b = cli_bootstrap(&claude_spec(&[]), Some(evil), None); + let b = cli_bootstrap(&claude_spec(&[]), Some(evil), None, false); assert!(!b.contains("rm -rf")); assert!(!b.contains("--resume")); } @@ -1797,14 +1867,14 @@ mod tests { // settings, so the model/sandbox/approval flags are NOT replayed. let id = "28b98f08-5f5f-4b1e-8c4e-41ae87c0c706"; let spec = codex_spec(&["-m", "gpt-5.5", "-s", "danger-full-access"]); - let b = cli_bootstrap(&spec, Some(id), None); + let b = cli_bootstrap(&spec, Some(id), None, false); assert!(b.contains(&format!("'codex' resume {id}"))); assert!( !b.contains("-m"), "base flags must not ride the resume: {b}" ); // Continue fallback uses `resume --last`, falling back to a fresh TUI. - let cont = cli_bootstrap(&spec, None, None); + let cont = cli_bootstrap(&spec, None, None, false); assert!(cont.contains("'codex' resume --last || 'codex'")); } @@ -1817,7 +1887,7 @@ mod tests { // word-split or parsed as shell (injection-safe by construction). let spec = claude_spec(&["--dangerously-skip-permissions"]); let file = Path::new("/tmp/vk/cli-prompts/abc.txt"); - let b = cli_bootstrap(&spec, None, Some(file)); + let b = cli_bootstrap(&spec, None, Some(file), false); assert!( b.len() < 2048, "bootstrap must stay small regardless of prompt size: {} bytes", @@ -1836,7 +1906,7 @@ mod tests { // embedded quote is POSIX-escaped as `'\''`, so the dangerous run stays // inert data inside the quoting rather than terminating it. let evil = Path::new("/tmp/'; rm -rf ~; echo '.txt"); - let b = cli_bootstrap(&spec, None, Some(evil)); + let b = cli_bootstrap(&spec, None, Some(evil), false); assert!( b.contains(r"'\''; rm -rf ~; echo '\''"), "path quote must be escaped, not terminated: {b}" @@ -1857,7 +1927,7 @@ mod tests { // the flag itself is quoted like every other word we emit. let flag_spec = CliLaunchSpec::new("gemini", vec![]) .with_prompt_arg(CliPromptArg::Flag("-i".to_string())); - let b = cli_bootstrap(&flag_spec, None, Some(file)); + let b = cli_bootstrap(&flag_spec, None, Some(file), false); assert!(b.contains("rm -f -- '/tmp/vk/p.txt'; 'gemini' '-i' \"$vk_p\"")); // StdinPipe agents pipe the file into the program — no argv ceiling. @@ -1865,7 +1935,7 @@ mod tests { // the file, so consumption is acknowledged (file gone) immediately — // not when the agent eventually exits. let pipe_spec = CliLaunchSpec::new("amp", vec![]).with_prompt_arg(CliPromptArg::StdinPipe); - let b = cli_bootstrap(&pipe_spec, None, Some(file)); + let b = cli_bootstrap(&pipe_spec, None, Some(file), false); assert!(b.contains("{ cat '/tmp/vk/p.txt'; rm -f -- '/tmp/vk/p.txt'; } | 'amp'")); } @@ -1874,11 +1944,47 @@ mod tests { // No prompt file (blank prompt filtered out by the caller) -> the // no-prompt continue/fresh path, exactly as before. let spec = claude_spec(&["--dangerously-skip-permissions"]); - let b = cli_bootstrap(&spec, None, None); + let b = cli_bootstrap(&spec, None, None, false); assert!(b.contains("--continue || 'claude'")); assert!(!b.contains("vk_p=")); } + #[test] + fn cli_bootstrap_deferred_prompt_launches_fresh_tui() { + // A paste delivery follows this launch: NO `--continue` (its doomed + // first leg on a brand-new workspace would live just long enough to + // swallow the paste and exit), just the bare agent TUI. + let spec = claude_spec(&["--dangerously-skip-permissions"]); + let b = cli_bootstrap(&spec, None, None, true); + assert!(!b.contains("--continue"), "no doomed continue leg: {b}"); + assert!(b.contains("'claude' '--dangerously-skip-permissions'")); + // Resume still wins over a pending deferred paste at launch time. + let id = "28b98f08-5f5f-4b1e-8c4e-41ae87c0c706"; + let b = cli_bootstrap(&spec, Some(id), None, true); + assert!(b.contains(&format!("--resume {id}"))); + } + + #[test] + fn route_followup_prompt_always_pastes_or_drops() { + // Follow-up delivery (live session / post-resume) is never baked. + assert_eq!( + route_followup_prompt(" keep going "), + CliPromptRouting::Deferred("keep going".to_string()) + ); + assert_eq!(route_followup_prompt(" \n\t"), CliPromptRouting::None); + } + + #[test] + fn multiline_or_oversized_text_takes_the_paste_transport() { + // `send-keys -l` types newlines as Enter keystrokes — a multi-line + // text would submit at its first line — so anything with a newline + // rides the bracketed-paste buffer path regardless of size. + assert!(!needs_paste_transport("single line")); + assert!(needs_paste_transport("two\nlines")); + assert!(needs_paste_transport(&"x".repeat(4096))); + assert!(!needs_paste_transport(&"x".repeat(4095))); + } + #[test] fn cli_prompt_file_content_matches_old_quoting_semantics() { // Blank (after trim) -> no file is written (falls through to continue). @@ -2018,22 +2124,32 @@ mod tests { } #[test] - fn shell_commands_are_recognized_for_paste_gating() { - // The deferred paste must never target a pane whose process tree is - // all shells (bootstrap still starting, or the missing-binary - // fallback shell). - for shell in ["sh", "bash", "zsh", "dash", "fish"] { - assert!(is_shell_command(shell), "{shell} must gate the paste"); - } - // Login-shell (`-zsh`) and full-path (`/bin/zsh`, macOS ps) spellings - // normalize to the same shell names. - assert!(is_shell_command("-zsh")); - assert!(is_shell_command("/bin/bash")); - assert!(is_shell_command(" -/usr/bin/fish".trim())); - // Agent TUIs (binary or interpreter names) unblock the paste. - for agent in ["claude", "codex", "node", "amp", "gemini"] { - assert!(!is_shell_command(agent), "{agent} must allow the paste"); + fn comm_matching_gates_paste_on_the_expected_agent_only() { + // The paste gate matches THE agent we launched, so a shell (bootstrap + // still starting / missing-binary fallback) — or an unrelated program + // the user ran in that fallback shell (vim, npm→node) — can never + // receive a prompt meant for the agent. + assert!(comm_matches_program("claude", "claude")); + assert!(comm_matches_program("codex", "codex")); + for not_agent in ["sh", "bash", "zsh", "vim", "node", "npm", "htop"] { + assert!( + !comm_matches_program(not_agent, "claude"), + "{not_agent} must not satisfy the claude gate" + ); } + // Full-path (macOS ps) and login-dash spellings normalize away. + assert!(comm_matches_program("/usr/local/bin/claude", "claude")); + assert!(comm_matches_program("-claude", "claude")); + // The kernel truncates comm to 15 bytes; long program names match on + // the truncated prefix. + assert!(comm_matches_program( + "verylongagentna", + "verylongagentname-cli" + )); + assert!(!comm_matches_program( + "verylongagentXX", + "verylongagentname-cli" + )); } #[test] @@ -2045,6 +2161,7 @@ mod tests { &claude_spec(&["--dangerously-skip-permissions"]), None, None, + false, ); assert!(b.contains( "'claude' '--dangerously-skip-permissions' --continue || 'claude' '--dangerously-skip-permissions'" @@ -2054,7 +2171,7 @@ mod tests { #[test] fn cli_bootstrap_shell_quotes_agent_args_on_every_form() { // Glob/metacharacters in a model id stay inert (single-quoted)... - let b = cli_bootstrap(&claude_spec(&["--model", "opus[1m]"]), None, None); + let b = cli_bootstrap(&claude_spec(&["--model", "opus[1m]"]), None, None, false); assert!(b.contains("'--model' 'opus[1m]'")); // ...and the flags ride the continue/fresh fallback too. assert!(b.contains("'opus[1m]' --continue")); @@ -2119,6 +2236,9 @@ mod tests { assert!(CLI_TMUX_CONF.contains("set -s set-clipboard on")); assert!(CLI_TMUX_CONF.contains("clipboard")); assert!(CLI_TMUX_CONF.contains("unbind-key -n MouseDown3Pane")); + // Window command strings (the POSIX-sh launch bootstrap) are parsed by + // default-shell; a fish/csh login shell would reject them outright. + assert!(CLI_TMUX_CONF.contains("set -g default-shell /bin/sh")); } #[test] diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 7341ce7a35..e2edef81c2 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -23,7 +23,7 @@ use executors::{ use local_deployment::pty::{ CLI_PROMPT_PARKED_NOTICE, CliPromptDelivery, CliPromptRouting, PtyCommand, cli_pane_agent_running, cli_prompt_file_exists, cli_tmux_available, cli_tmux_session_exists, - kill_cli_tmux_session, remove_cli_prompt_file, route_initial_prompt, send_cli_keys, + remove_cli_prompt_file, route_followup_prompt, route_initial_prompt, send_cli_keys, }; use serde::{Deserialize, Serialize}; use sqlx::SqlitePool; @@ -270,11 +270,23 @@ async fn terminal_ws( // Since availability is process-cached, `true` here means // `create_session` also takes the tmux branch, so a successful // spawn really did carry the prompt into a tmux session. - let carried: Option<(String, CliPromptDelivery, Uuid)> = match &session { - Some(s) if cli_tmux_available() => { - match CliPromptDelivery::try_claim(query.workspace_id) { - Some(claim) => match Session::peek_pending_cli_prompt(pool, s.id).await { - Ok(Some(prompt)) => Some((prompt, claim, s.id)), + // Workspace-scoped peek: creation parks on the CLI-first session, + // loop wake-ups re-park on the LATEST session, and this attach may + // have resolved a third (frontend-selected) session — a + // session-scoped peek would strand a prompt parked on a sibling + // row. + let carried: Option<(String, CliPromptDelivery, Uuid)> = if cli_tmux_available() { + match CliPromptDelivery::try_claim(query.workspace_id) { + Some(claim) => { + match Session::peek_pending_cli_prompt_for_workspace( + pool, + query.workspace_id, + ) + .await + { + Ok(Some((owning_session_id, prompt))) => { + Some((prompt, claim, owning_session_id)) + } // Nothing parked: release the claim (drop). Ok(None) => None, Err(e) => { @@ -283,24 +295,25 @@ async fn terminal_ws( // error here delays delivery, so make it // observable. tracing::warn!( - "Failed to read pending CLI prompt for session {}: {}", - s.id, + "Failed to read pending CLI prompt for workspace {}: {}", + query.workspace_id, e ); None } - }, - None => { - tracing::debug!( - "CLI prompt delivery for workspace {} already in flight; \ - attaching without the prompt", - query.workspace_id - ); - None } } + None => { + tracing::debug!( + "CLI prompt delivery for workspace {} already in flight; \ + attaching without the prompt", + query.workspace_id + ); + None + } } - _ => None, + } else { + None }; // Honor the workspace's selected agent + model/effort at launch // (defaults to claude at Opus/max when nothing was selected). @@ -321,23 +334,32 @@ async fn terminal_ws( // Either way the parked prompt is cleared only after delivery is // confirmed. let mut baked_prompt = None; + let mut deferred_prompt_pending = false; if let Some((peeked, claim, clear_session_id)) = carried { let fresh_launch = resume_session_id.is_none() && !cli_tmux_session_exists(query.workspace_id).await; let routed = if fresh_launch { route_initial_prompt(Some(peeked.clone()), &spec.prompt_arg) } else { - let trimmed = peeked.trim(); - if trimmed.is_empty() { - CliPromptRouting::None - } else { - CliPromptRouting::Deferred(trimmed.to_string()) - } + route_followup_prompt(&peeked) }; match routed { - // Blank-after-trim: nothing will be delivered; the claim - // drops here rather than being held across the attach. - CliPromptRouting::None => {} + CliPromptRouting::None => { + // Blank-after-trim: nothing can ever be delivered, so + // clear the parked blank (CAS keeps a newer prompt + // safe) instead of re-claiming and re-probing it on + // every future attach. The claim drops here. + match Session::clear_pending_cli_prompt(pool, clear_session_id, &peeked) + .await + { + Ok(_) => {} + Err(e) => tracing::warn!( + "Failed to clear blank parked CLI prompt for session {}: {}", + clear_session_id, + e + ), + } + } CliPromptRouting::Baked(prompt) => { baked_prompt = Some(prompt); prompt_delivery = Some(PromptDelivery { @@ -345,15 +367,21 @@ async fn terminal_ws( clear_session_id, peeked, deferred: None, + program: spec.program.clone(), claim, }); } CliPromptRouting::Deferred(prompt) => { + // Only a FRESH launch needs the bare-TUI bootstrap; + // for an existing session (or resume) the bootstrap + // is ignored / resume wins. + deferred_prompt_pending = fresh_launch; prompt_delivery = Some(PromptDelivery { workspace_id: query.workspace_id, clear_session_id, peeked, deferred: Some(prompt), + program: spec.program.clone(), claim, }); } @@ -366,6 +394,7 @@ async fn terminal_ws( workspace_id: query.workspace_id, resume_session_id, initial_prompt: baked_prompt, + deferred_prompt_pending, spec, }, ) @@ -425,6 +454,10 @@ struct PromptDelivery { /// Prompt to paste once the agent owns the pane (`None` = baked into the /// launch's temp-file transport instead). deferred: Option, + /// The agent binary this launch runs — delivery is only ever confirmed + /// against THIS process owning the pane, so a fallback shell (or anything + /// the user runs inside it) can never satisfy the gate. + program: String, /// Exclusive in-process claim; held until the parked copy is cleared or /// deliberately left parked, releasing on drop. claim: CliPromptDelivery, @@ -476,8 +509,10 @@ async fn handle_terminal_ws( // confirm-then-clear window. let _claim = delivery.claim; let delivered = match &delivery.deferred { - Some(text) => deliver_deferred_prompt(workspace_id, text).await, - None => confirm_baked_prompt_consumed(workspace_id).await, + Some(text) => { + deliver_deferred_prompt(workspace_id, text, &delivery.program).await + } + None => confirm_baked_prompt_consumed(workspace_id, &delivery.program).await, }; if delivered { match Session::clear_pending_cli_prompt( @@ -513,14 +548,15 @@ async fn handle_terminal_ws( } }); } else { - // The session never appeared — tear down whatever half-state the - // launch left behind and surface the failure. `kill_cli_tmux_session` - // both removes the transient prompt file (the DB copy stays parked) - // and kills a session that squeaked in after the poll window, so the - // next attach deterministically re-peeks the parked prompt instead - // of finding a stray session that blocks delivery. The frontend - // renders the error in red and halts its reconnect loop. - kill_cli_tmux_session(workspace_id).await; + // The session never appeared — remove the transient prompt file + // (the DB copy stays parked) and surface the failure. Deliberately + // NOT killing a session that squeaks in after the poll window: a + // slow-but-successful launch may already be running the agent (or + // hosting another attach's client), and murdering it loses real + // work — while a parked prompt behind a live session is now + // recoverable anyway (the next attach delivers it by paste). The + // frontend renders the error in red and halts its reconnect loop. + remove_cli_prompt_file(workspace_id); tracing::error!( "CLI tmux session for workspace {} never came up; prompt left parked", workspace_id @@ -609,40 +645,57 @@ async fn wait_for_cli_session(workspace_id: Uuid) -> bool { /// Confirm a baked prompt was actually handed to the agent: the staged file is /// gone (the bootstrap `rm`s it at hand-off — see [`cli_prompt_file_exists`]) -/// AND a non-shell process owns the pane. The second condition catches the -/// consumed-but-never-executed window (`command -v` passed but the agent's +/// AND the expected agent process owns the pane. The second condition catches +/// the consumed-but-never-executed window (`command -v` passed but the agent's /// exec failed: bad shebang, loader error, permissions) where file-gone alone /// would clear a prompt no agent ever received. An unconfirmed launch leaves -/// the parked DB prompt for the next attach's paste/fresh-launch retry. -async fn confirm_baked_prompt_consumed(workspace_id: Uuid) -> bool { - for _ in 0..20 { +/// the parked DB prompt for the next attach's paste/fresh-launch retry — and +/// drops the never-consumed file so a later launch can't half-consume it. +async fn confirm_baked_prompt_consumed(workspace_id: Uuid, program: &str) -> bool { + for _ in 0..30 { if !cli_prompt_file_exists(workspace_id) - && cli_pane_agent_running(workspace_id).await == Some(true) + && cli_pane_agent_running(workspace_id, program).await == Some(true) { return true; } - tokio::time::sleep(std::time::Duration::from_millis(200)).await; + tokio::time::sleep(std::time::Duration::from_millis(250)).await; } + remove_cli_prompt_file(workspace_id); false } -/// Deliver an oversized (non-inlineable) prompt by paste once the AGENT owns -/// the pane. Pasting earlier would hand the prompt to the bootstrap/fallback -/// shell instead — which executes it as shell input (binary missing) or -/// truncates it at the tty's canonical-mode line limit (TUI not yet in raw -/// mode). Readiness = a non-shell process in the pane on two consecutive -/// polls (a single read could catch the short-lived first leg of the -/// `--continue || fresh` relaunch), plus a short grace for the TUI to enter -/// raw mode. Bounded at ~10s; an unready pane leaves the prompt parked. -async fn deliver_deferred_prompt(workspace_id: Uuid, text: &str) -> bool { +/// Deliver a deferred prompt by paste once THE AGENT owns the pane. Pasting +/// earlier would hand the prompt to the bootstrap/fallback shell instead — +/// which executes it as shell input (binary missing) or truncates it at the +/// tty's canonical-mode line limit (TUI not yet in raw mode). Readiness = the +/// expected agent process in the pane on two consecutive polls, re-checked +/// right before the paste (the agent can die inside the grace window — e.g. a +/// failed resume attempt — and the pane fall back to a shell) and re-checked +/// after it (an agent that exited immediately after the paste discarded the +/// text with its tty; delivery must not be confirmed). Bounded at ~15s; an +/// unready pane leaves the prompt parked for the next attach's retry. +async fn deliver_deferred_prompt(workspace_id: Uuid, text: &str, program: &str) -> bool { let mut stable = 0u32; - for _ in 0..40 { - match cli_pane_agent_running(workspace_id).await { + for _ in 0..60 { + match cli_pane_agent_running(workspace_id, program).await { Some(true) => { stable += 1; if stable >= 2 { + // Grace for the TUI to enter raw mode, then re-verify the + // agent still owns the pane immediately before pasting. tokio::time::sleep(std::time::Duration::from_millis(750)).await; - return send_cli_keys(workspace_id, text).await; + if cli_pane_agent_running(workspace_id, program).await != Some(true) { + stable = 0; + continue; + } + if !send_cli_keys(workspace_id, text).await { + return false; + } + // Post-paste ack: the agent must have survived receiving + // it. A process that died right after the paste (doomed + // resume leg, instant crash) never processed the text. + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + return cli_pane_agent_running(workspace_id, program).await == Some(true); } } Some(false) => stable = 0, @@ -674,3 +727,160 @@ async fn send_error(socket: &mut MaybeSignedWebSocket, message: &str) -> anyhow: pub(super) fn router() -> Router { Router::new().route("/terminal/ws", get(terminal_ws)) } + +#[cfg(test)] +mod tests { + use db::models::session::Session; + use sqlx::SqlitePool; + use uuid::Uuid; + + /// Minimal in-memory slice of the `sessions` table — just the columns the + /// parked-prompt primitives touch — so the CAS/park semantics that guard + /// the "never destroy the prompt" invariant are exercised against real + /// SQLite. (Lives here rather than in `crates/db` because the db crate + /// has no async test runtime; this route module is the consumer whose + /// correctness depends on these semantics.) + async fn pool_with_session(id: Uuid, workspace_id: Uuid) -> SqlitePool { + let pool = SqlitePool::connect("sqlite::memory:").await.unwrap(); + sqlx::query( + "CREATE TABLE sessions ( + id BLOB PRIMARY KEY, + workspace_id BLOB NOT NULL, + pending_cli_prompt TEXT, + created_at TEXT NOT NULL DEFAULT (datetime('now', 'subsec')) + )", + ) + .execute(&pool) + .await + .unwrap(); + sqlx::query("INSERT INTO sessions (id, workspace_id) VALUES ($1, $2)") + .bind(id) + .bind(workspace_id) + .execute(&pool) + .await + .unwrap(); + pool + } + + #[tokio::test] + async fn clear_pending_cli_prompt_is_compare_and_swap() { + let sid = Uuid::new_v4(); + let pool = pool_with_session(sid, Uuid::new_v4()).await; + + // Park, then clear with the exact delivered value: clears. + Session::set_pending_cli_prompt(&pool, sid, "deliver me") + .await + .unwrap(); + assert!( + Session::clear_pending_cli_prompt(&pool, sid, "deliver me") + .await + .unwrap(), + "matching clear must succeed" + ); + assert_eq!( + Session::peek_pending_cli_prompt(&pool, sid).await.unwrap(), + None + ); + + // A NEWER prompt parked mid-confirmation survives the older + // delivery's clear (the CAS misses) — the invariant this method + // exists for. + Session::set_pending_cli_prompt(&pool, sid, "newer prompt") + .await + .unwrap(); + assert!( + !Session::clear_pending_cli_prompt(&pool, sid, "deliver me") + .await + .unwrap(), + "stale clear must be superseded" + ); + assert_eq!( + Session::peek_pending_cli_prompt(&pool, sid).await.unwrap(), + Some("newer prompt".to_string()) + ); + + // Clearing an empty slot is a no-op, not an error. + Session::clear_pending_cli_prompt(&pool, sid, "newer prompt") + .await + .unwrap(); + assert!( + !Session::clear_pending_cli_prompt(&pool, sid, "newer prompt") + .await + .unwrap() + ); + } + + #[tokio::test] + async fn repark_only_fills_an_empty_slot() { + let sid = Uuid::new_v4(); + let pool = pool_with_session(sid, Uuid::new_v4()).await; + + // Empty slot: the wake-up parks. + assert!( + Session::set_pending_cli_prompt_if_empty(&pool, sid, "continue") + .await + .unwrap() + ); + // Occupied slot: a wake-up must never overwrite a parked prompt. + assert!( + !Session::set_pending_cli_prompt_if_empty(&pool, sid, "boilerplate") + .await + .unwrap() + ); + assert_eq!( + Session::peek_pending_cli_prompt(&pool, sid).await.unwrap(), + Some("continue".to_string()) + ); + } + + #[tokio::test] + async fn workspace_peek_finds_prompt_on_any_session_row() { + let workspace_id = Uuid::new_v4(); + let older = Uuid::new_v4(); + let pool = pool_with_session(older, workspace_id).await; + // A second, newer session in the same workspace (distinct created_at + // ordering via explicit timestamps). + let newer = Uuid::new_v4(); + sqlx::query( + "INSERT INTO sessions (id, workspace_id, created_at) + VALUES ($1, $2, datetime('now', '+1 hour'))", + ) + .bind(newer) + .bind(workspace_id) + .execute(&pool) + .await + .unwrap(); + + // Prompt parked on the OLDER (e.g. CLI-first) session is still found + // when the attach resolved the newer session. + Session::set_pending_cli_prompt(&pool, older, "parked on older") + .await + .unwrap(); + assert_eq!( + Session::peek_pending_cli_prompt_for_workspace(&pool, workspace_id) + .await + .unwrap(), + Some((older, "parked on older".to_string())) + ); + + // With prompts on both rows, the newest session's wins (loop re-parks + // land on the latest session). + Session::set_pending_cli_prompt(&pool, newer, "parked on newer") + .await + .unwrap(); + assert_eq!( + Session::peek_pending_cli_prompt_for_workspace(&pool, workspace_id) + .await + .unwrap(), + Some((newer, "parked on newer".to_string())) + ); + + // Foreign workspaces see nothing. + assert_eq!( + Session::peek_pending_cli_prompt_for_workspace(&pool, Uuid::new_v4()) + .await + .unwrap(), + None + ); + } +} From 9197cd4d42d5c141ac7e211a141da49d7d4bd16a Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 22:51:51 +0000 Subject: [PATCH 07/14] fix(cli-mode): confirm delivery through node-wrapped agents via pane subtree walk The delivery gate cli_pane_agent_running only inspected the pane root and its direct children, and its doc claimed a shebang exec sets comm to the script basename. Both are wrong for node-wrapped agents: codex ships as a '#!/usr/bin/env node' launcher whose comm is 'node' and which spawns the native codex as a GRANDCHILD (sh -> node -> codex, verified on a scratch socket). The gate therefore never confirmed codex delivery, so every baked codex prompt was delivered via argv but left parked -> replayed on the next fresh launch, and >100KB codex prompts (deferred paste) never delivered at all. Walk the whole pane process subtree from a single 'ps' snapshot (pure, unit- tested helper) and match the exact program name at any depth; still refuse the intermediate 'node' and any fallback-shell program, so a user's vim/npm can't satisfy the gate. Also force 0600 on the prompt file after open (a pre-existing looser-perm file would otherwise keep its mode; the 0700 dir is the real control, this is defense in depth) and drop the now-unused process_comm helper. --- crates/local-deployment/src/pty.rs | 155 ++++++++++++++++++++--------- 1 file changed, 107 insertions(+), 48 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 220c2d1279..d0851a573b 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -330,13 +330,19 @@ fn write_cli_prompt_file(workspace_id: Uuid, content: &str) -> std::io::Result

bool { } /// Whether THE EXPECTED AGENT (`program`, the spec's binary name) currently -/// owns a workspace's CLI pane. `None` when the pane itself can't be read -/// (session gone). +/// runs anywhere in a workspace's CLI pane process tree. `None` when the pane +/// itself can't be read (session gone). /// /// tmux runs our bootstrap string via `default-shell -c`, and every launch /// stage shares that shell's process group — so `#{pane_current_command}` /// reports the outer shell for the pane's whole life (verified empirically) /// and can't distinguish "agent running" from "fallback shell". Instead this -/// inspects the pane's process tree: the pane root (`#{pane_pid}`) or one of -/// its direct children must BE the expected agent. Matching the exact program -/// name (shebang exec sets the process comm to the script basename — verified -/// on a scratch socket) rather than "any non-shell" means a user running -/// vim/npm inside the missing-binary fallback shell can never satisfy the -/// paste gate and receive a prompt meant for the agent. +/// walks the pane's process SUBTREE (from `#{pane_pid}` down) looking for a +/// process whose comm IS the expected agent. +/// +/// The whole subtree, not just the pane root and its direct children, because +/// node-wrapped agents run the native binary as a GRANDCHILD: `codex` ships as +/// a `#!/usr/bin/env node` launcher that `spawn`s the native `codex` binary, so +/// the pane tree is `sh → node → codex`. A shebang exec sets comm to the +/// interpreter (`node`), NOT the script basename (verified on a scratch +/// socket), and we deliberately do NOT accept the intermediate `node` as an +/// agent — so a direct-children-only probe would never confirm a node-wrapped +/// agent's delivery, stranding its parked prompt for replay on the next fresh +/// launch. Matching the EXACT program name (not "any non-shell") still keeps a +/// user's vim/npm inside the missing-binary fallback shell from satisfying the +/// paste gate. pub async fn cli_pane_agent_running(workspace_id: Uuid, program: &str) -> Option { if !tmux_available() { return None; @@ -967,33 +981,60 @@ pub async fn cli_pane_agent_running(workspace_id: Uuid, program: &str) -> Option .parse::() .ok()?; - // Direct children of the pane shell first — the agent is a direct child - // in every current launch shape, so this is the probe that usually - // answers. (`pgrep -l -P` prints " "; a non-zero exit just - // means no children — the bootstrap is between stages or the fallback - // shell is idle.) - let children = tokio::process::Command::new("pgrep") - .args(["-l", "-P", &pane_pid.to_string()]) + // One process snapshot, walked in-process: a single `ps` instead of an + // unbounded fan-out of `pgrep -P` calls, and a consistent view of the tree. + let snapshot = tokio::process::Command::new("ps") + .args(["-eo", "pid=,ppid=,comm="]) .stderr(std::process::Stdio::null()) .output() .await .ok()?; - let listing = String::from_utf8_lossy(&children.stdout); - if listing - .lines() - .filter_map(|line| line.split_whitespace().nth(1)) - .any(|comm| comm_matches_program(comm, program)) - { - return Some(true); + if !snapshot.status.success() { + return None; } + let listing = String::from_utf8_lossy(&snapshot.stdout); + Some(pane_subtree_has_program(&listing, pane_pid, program)) +} - // Fall back to the pane root: normally the bootstrap/fallback shell, but - // a future exec'd agent would show up here directly. - Some( - process_comm(pane_pid) - .await - .is_some_and(|comm| comm_matches_program(&comm, program)), - ) +/// Whether `program` runs anywhere in the process subtree rooted at `root_pid` +/// (inclusive), given a `ps -eo pid=,ppid=,comm=` snapshot. Pure so the tree +/// walk — the part that decides whether a node-wrapped agent grandchild counts +/// — is unit testable without a live process tree. +fn pane_subtree_has_program(ps_listing: &str, root_pid: u32, program: &str) -> bool { + let mut comm_by_pid: HashMap = HashMap::new(); + let mut children: HashMap> = HashMap::new(); + for line in ps_listing.lines() { + let mut fields = line.split_whitespace(); + let (Some(pid), Some(ppid), Some(comm)) = (fields.next(), fields.next(), fields.next()) + else { + continue; + }; + let (Ok(pid), Ok(ppid)) = (pid.parse::(), ppid.parse::()) else { + continue; + }; + comm_by_pid.insert(pid, comm); + children.entry(ppid).or_default().push(pid); + } + // Depth-first from the pane root, inclusive. `visited` guards against a + // malformed snapshot: a real ppid graph is a forest and can't cycle, but a + // torn read must not spin. + let mut stack = vec![root_pid]; + let mut visited = HashSet::new(); + while let Some(pid) = stack.pop() { + if !visited.insert(pid) { + continue; + } + if comm_by_pid + .get(&pid) + .is_some_and(|comm| comm_matches_program(comm, program)) + { + return true; + } + if let Some(kids) = children.get(&pid) { + stack.extend(kids); + } + } + false } /// Whether a `ps`/`pgrep` process name is the expected agent binary. The @@ -1007,24 +1048,8 @@ fn comm_matches_program(comm: &str, program: &str) -> bool { || (program.len() > 15 && program.get(..15).is_some_and(|prefix| comm == prefix)) } -/// Best-effort process name for a pid (`ps -o comm=`), raw (trimmed only — -/// [`is_shell_command`] owns normalization). -async fn process_comm(pid: u32) -> Option { - let output = tokio::process::Command::new("ps") - .args(["-o", "comm=", "-p", &pid.to_string()]) - .stderr(std::process::Stdio::null()) - .output() - .await - .ok()?; - if !output.status.success() { - return None; - } - let comm = String::from_utf8_lossy(&output.stdout).trim().to_string(); - (!comm.is_empty()).then_some(comm) -} - -/// Normalize a `ps`/`pgrep` process name for comparison: login shells report -/// as `-zsh`, and macOS `ps -o comm=` can report a full path. +/// Normalize a `ps` process name for comparison: login shells report as +/// `-zsh`, and macOS `ps -o comm=` can report a full path. fn normalize_comm(comm: &str) -> &str { let comm = comm.trim().trim_start_matches('-'); comm.rsplit('/').next().unwrap_or(comm) @@ -2152,6 +2177,40 @@ mod tests { )); } + #[test] + fn pane_subtree_finds_node_wrapped_agent_grandchild() { + // codex ships as `#!/usr/bin/env node` which spawns the native `codex` + // as a GRANDCHILD, so the pane tree is `sh(pane) → node → codex`. The + // gate must descend past the intermediate `node` (which is NOT an + // agent) to confirm delivery — a direct-children-only probe would miss + // it and strand the prompt for replay. + let ps = "\ + 100 1 sh + 200 100 node + 300 200 codex + 400 1 unrelated + 500 400 vim +"; + assert!( + pane_subtree_has_program(ps, 100, "codex"), + "grandchild agent under a node wrapper must be found" + ); + // Native agent as a direct child (claude is an ELF binary). + let ps_native = " 100 1 sh\n 200 100 claude\n"; + assert!(pane_subtree_has_program(ps_native, 100, "claude")); + + // A shell-only subtree (bootstrap still starting / missing-binary + // fallback) must NOT satisfy the gate... + assert!(!pane_subtree_has_program(" 100 1 sh\n", 100, "codex")); + // ...nor may an unrelated program the user ran in the fallback shell + // (node from an `npm` invocation is exactly the intermediate we refuse + // to accept as the agent). + let ps_npm = " 100 1 sh\n 200 100 node\n 300 200 esbuild\n"; + assert!(!pane_subtree_has_program(ps_npm, 100, "codex")); + // A sibling subtree's agent (different pane) is out of scope. + assert!(!pane_subtree_has_program(ps, 400, "codex")); + } + #[test] fn cli_bootstrap_falls_back_to_continue_then_fresh() { // With nothing explicit to run: continue the cwd's latest conversation From 3e516487216b3cda130514a4165420a14cd6754a Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 22:52:01 +0000 Subject: [PATCH 08/14] fix(cli-mode): paste-recover a baked prompt when a racing attach wins session creation Two racing first-attaches to a brand-new workspace both run 'tmux new-session -A'. The prompt-carrying attach (claim winner) can compute fresh_launch=true and bake the prompt into its bootstrap, yet the claim LOSER (promptless) can reach new-session first and create the session with a fresh/continue bootstrap; the winner's '-A' attach then just joins that session and its baked bootstrap is ignored. Confirmation then timed out, dropped the file, and left the prompt parked until a later attach -- the live agent started with no initial prompt. confirm_or_recover_baked_prompt now, on confirmation timeout, pastes the prompt into whatever agent owns the pane IF the staged file is still on disk. For argv agents (Positional/Flag) the bootstrap rm's the file before exec, so a surviving file proves our bootstrap never ran and the prompt was never argv-delivered -- pasting can't double-deliver. StdinPipe/Unsupported carry no fallback (a lingering file there can mean cat is mid-stream). File cleanup moves to the recovery function so the confirmed path never yanks a file out from under a consumer. --- crates/server/src/routes/terminal.rs | 85 ++++++++++++++++++++++++---- 1 file changed, 73 insertions(+), 12 deletions(-) diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index e2edef81c2..fc859a025c 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -17,7 +17,10 @@ use db::models::{ use deployment::Deployment; use executors::{ actions::ExecutorActionType, - executors::{BaseCodingAgent, StandardCodingAgentExecutor, cli::CliLaunchSpec}, + executors::{ + BaseCodingAgent, StandardCodingAgentExecutor, + cli::{CliLaunchSpec, CliPromptArg}, + }, profile::{ExecutorConfig, ExecutorConfigs}, }; use local_deployment::pty::{ @@ -361,12 +364,26 @@ async fn terminal_ws( } } CliPromptRouting::Baked(prompt) => { + // Argv agents (Positional/Flag) `rm` the staged file + // before exec, so a file left on disk after + // confirmation fails proves the bootstrap never ran and + // the prompt can be safely pasted as a recovery. + // StdinPipe carries no fallback (a lingering file can + // mean `cat` is still streaming — pasting would + // double-deliver). + let baked_paste_fallback = match spec.prompt_arg { + CliPromptArg::StdinPipe | CliPromptArg::Unsupported => None, + CliPromptArg::Positional | CliPromptArg::Flag(_) => { + Some(prompt.clone()) + } + }; baked_prompt = Some(prompt); prompt_delivery = Some(PromptDelivery { workspace_id: query.workspace_id, clear_session_id, peeked, deferred: None, + baked_paste_fallback, program: spec.program.clone(), claim, }); @@ -381,6 +398,7 @@ async fn terminal_ws( clear_session_id, peeked, deferred: Some(prompt), + baked_paste_fallback: None, program: spec.program.clone(), claim, }); @@ -454,6 +472,16 @@ struct PromptDelivery { /// Prompt to paste once the agent owns the pane (`None` = baked into the /// launch's temp-file transport instead). deferred: Option, + /// For a BAKED delivery only: the trimmed prompt to paste as a recovery + /// fallback if the baked bootstrap turns out never to have run (a racing + /// attach won `new-session -A` and created the pane with a promptless + /// launch, so our `-A` attach's baked bootstrap was ignored). `Some` only + /// for argv agents (`Positional`/`Flag`), whose bootstrap `rm`s the file + /// BEFORE exec — so a file still on disk proves the prompt was never + /// argv-delivered and pasting can't double-deliver. `None` for `StdinPipe` + /// (a lingering file there can mean `cat` is mid-stream) and for deferred + /// deliveries (which paste anyway). + baked_paste_fallback: Option, /// The agent binary this launch runs — delivery is only ever confirmed /// against THIS process owning the pane, so a fallback shell (or anything /// the user runs inside it) can never satisfy the gate. @@ -512,7 +540,14 @@ async fn handle_terminal_ws( Some(text) => { deliver_deferred_prompt(workspace_id, text, &delivery.program).await } - None => confirm_baked_prompt_consumed(workspace_id, &delivery.program).await, + None => { + confirm_or_recover_baked_prompt( + workspace_id, + &delivery.program, + delivery.baked_paste_fallback.as_deref(), + ) + .await + } }; if delivered { match Session::clear_pending_cli_prompt( @@ -643,15 +678,32 @@ async fn wait_for_cli_session(workspace_id: Uuid) -> bool { false } -/// Confirm a baked prompt was actually handed to the agent: the staged file is -/// gone (the bootstrap `rm`s it at hand-off — see [`cli_prompt_file_exists`]) -/// AND the expected agent process owns the pane. The second condition catches -/// the consumed-but-never-executed window (`command -v` passed but the agent's -/// exec failed: bad shebang, loader error, permissions) where file-gone alone -/// would clear a prompt no agent ever received. An unconfirmed launch leaves -/// the parked DB prompt for the next attach's paste/fresh-launch retry — and -/// drops the never-consumed file so a later launch can't half-consume it. -async fn confirm_baked_prompt_consumed(workspace_id: Uuid, program: &str) -> bool { +/// Confirm a baked prompt reached the agent, recovering the racing-attach case. +/// +/// Normal confirmation: the staged file is gone (the bootstrap `rm`s it at +/// hand-off — see [`cli_prompt_file_exists`]) AND the expected agent owns the +/// pane. The agent-running condition catches the consumed-but-never-executed +/// window (`command -v` passed but the agent's exec failed: bad shebang, loader +/// error, permissions) where file-gone alone would clear a prompt no agent ever +/// received. +/// +/// Recovery: if confirmation times out but the file is STILL on disk and +/// `paste_fallback` is set (argv agents `rm` the file before exec, so a +/// surviving file proves our baked bootstrap never ran — typically because a +/// racing attach won `new-session -A` and created the pane with a promptless +/// launch, leaving our `-A` attach's bootstrap ignored), paste the prompt into +/// whatever agent now owns the pane instead of stranding it for a future +/// attach. Pasting is safe precisely because the file survived: the prompt was +/// never argv-delivered, so it can't double-deliver. +/// +/// Either way the never-consumed file is dropped so a later launch can't +/// half-consume it; an unconfirmed, unrecovered launch leaves the parked DB +/// prompt for the next attach's retry. +async fn confirm_or_recover_baked_prompt( + workspace_id: Uuid, + program: &str, + paste_fallback: Option<&str>, +) -> bool { for _ in 0..30 { if !cli_prompt_file_exists(workspace_id) && cli_pane_agent_running(workspace_id, program).await == Some(true) @@ -660,8 +712,17 @@ async fn confirm_baked_prompt_consumed(workspace_id: Uuid, program: &str) -> boo } tokio::time::sleep(std::time::Duration::from_millis(250)).await; } + // Confirmation exhausted. A surviving file + an argv agent means the baked + // bootstrap never ran (a racing attach created the session first): paste + // the prompt into the live agent as a recovery. + let recovered = match paste_fallback { + Some(text) if cli_prompt_file_exists(workspace_id) => { + deliver_deferred_prompt(workspace_id, text, program).await + } + _ => false, + }; remove_cli_prompt_file(workspace_id); - false + recovered } /// Deliver a deferred prompt by paste once THE AGENT owns the pane. Pasting From 572085f69f38858b3edf648c1862bffe44849daf Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 22:52:15 +0000 Subject: [PATCH 09/14] fix(loop-automation): workspace-scoped re-park guard; keep wake-up pending on transient DB error Two gaps in the wake-up re-park recovery: 1. repark_prompt only checked the LATEST session row's empty slot (set_pending_cli_prompt_if_empty), but attach delivery peeks the workspace's newest pending prompt across ALL session rows. With an older session holding an undelivered user prompt and a newer empty session, a wake-up parked continuation boilerplate on the newer row and it delivered BEFORE the user prompt. Guard on a workspace-scoped peek so a prompt parked anywhere blocks re-park (the user prompt delivers first; the loop re-detects its banner). 2. Both failure branches called the best-effort repark_prompt (returning ()) and then marked the wake-up fired, so a transient DB error during re-park dropped the wake-up entirely -- neither delivered nor parked nor pending. repark_prompt now returns a ReparkOutcome; only Failed (a transient DB error) leaves the wake-up pending for the next tick, mirroring the existing capture-failure branch. Handled/NoSession stay final. --- .../local-deployment/src/loop_supervisor.rs | 83 +++++++++++++++---- 1 file changed, 69 insertions(+), 14 deletions(-) diff --git a/crates/local-deployment/src/loop_supervisor.rs b/crates/local-deployment/src/loop_supervisor.rs index b449413f9b..4941aa73ab 100644 --- a/crates/local-deployment/src/loop_supervisor.rs +++ b/crates/local-deployment/src/loop_supervisor.rs @@ -416,7 +416,16 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), // The tmux session may have been reaped before a long usage-window wake // came due — re-park the prompt so the next attach delivers it. if !cli_tmux_session_exists(wid).await { - repark_prompt(pool, wid, &prompt).await; + if repark_prompt(pool, wid, &prompt).await == ReparkOutcome::Failed { + // Transient DB error: leave the wake-up pending so the next + // tick retries rather than dropping it (marking it fired here + // would neither deliver nor park the prompt). + tracing::warn!( + "loop: workspace {wid} session gone; re-park failed transiently, \ + leaving wake-up pending" + ); + continue; + } tracing::info!("loop: workspace {wid} session gone; re-parked wake-up prompt"); ScheduledWakeup::mark_fired(pool, wakeup.id).await?; continue; @@ -470,40 +479,86 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), // prompt so the next terminal attach delivers it rather than dropping // the wake-up — mirrors the session-gone branch above. tracing::warn!("loop: failed to deliver wake-up to workspace {wid}; re-parking"); - repark_prompt(pool, wid, &prompt).await; + if repark_prompt(pool, wid, &prompt).await == ReparkOutcome::Failed { + // Transient DB error: leave the wake-up pending for the next + // tick instead of marking it fired and dropping it. + tracing::warn!( + "loop: re-park failed transiently for workspace {wid}; \ + leaving wake-up pending" + ); + continue; + } } ScheduledWakeup::mark_fired(pool, wakeup.id).await?; } Ok(()) } +/// Outcome of a [`repark_prompt`] attempt. Only `Failed` (a transient DB error) +/// warrants leaving the wake-up pending for a retry; every other outcome is +/// final and the caller may mark the wake-up fired. +#[derive(Debug, PartialEq, Eq)] +enum ReparkOutcome { + /// The prompt is parked (or a prompt is already parked for the workspace, + /// which delivers first) — handled, mark the wake-up fired. + Handled, + /// No session row exists to park on — nothing more the loop can do. + NoSession, + /// A transient DB error — leave the wake-up pending so the next tick retries + /// rather than silently dropping it. + Failed, +} + /// Best-effort: park `prompt` on the workspace's latest session so a terminal /// attach delivers it (the attach path pastes a parked prompt into a live /// agent pane, or hands it to the next fresh launch). Shared by both wake-up -/// failure branches so the re-park policy has exactly one owner. Parks only -/// into an EMPTY slot: continuation boilerplate must never overwrite a -/// parked-but-undelivered user prompt (that prompt delivers first, and the -/// loop re-detects its limit banner afterwards). -async fn repark_prompt(pool: &sqlx::SqlitePool, wid: Uuid, prompt: &str) { +/// failure branches so the re-park policy has exactly one owner. +/// +/// Parks only when the workspace has NO undelivered prompt anywhere: attach +/// delivery peeks the workspace's newest pending prompt across ALL its session +/// rows ([`Session::peek_pending_cli_prompt_for_workspace`]), so parking +/// continuation boilerplate onto a newer, empty session while an OLDER session +/// still holds an undelivered user prompt would let the continuation be +/// delivered first. Skipping keeps the user prompt ahead; the loop re-detects +/// its limit banner afterwards. +async fn repark_prompt(pool: &sqlx::SqlitePool, wid: Uuid, prompt: &str) -> ReparkOutcome { + // Workspace-scoped guard, matching the workspace-scoped delivery peek — not + // just the latest session row's slot (which `set_pending_cli_prompt_if_empty` + // checks): a prompt parked on ANY row means one is already queued to deliver. + match Session::peek_pending_cli_prompt_for_workspace(pool, wid).await { + Ok(Some(_)) => { + tracing::info!( + "loop: workspace {wid} already has a parked prompt; wake-up not re-parked" + ); + return ReparkOutcome::Handled; + } + Ok(None) => {} + Err(e) => { + tracing::warn!("loop: failed to check parked prompt for workspace {wid}: {e}"); + return ReparkOutcome::Failed; + } + } match Session::find_latest_by_workspace_id(pool, wid).await { Ok(Some(session)) => { match Session::set_pending_cli_prompt_if_empty(pool, session.id, prompt).await { - Ok(true) => {} - Ok(false) => tracing::info!( - "loop: workspace {wid} already has a parked prompt; wake-up not re-parked" - ), + // `false` = a prompt was parked between our check and here + // (raced an attach/another wake-up); either way one is queued. + Ok(_) => ReparkOutcome::Handled, Err(e) => { tracing::warn!( "loop: failed to re-park wake-up prompt for workspace {wid}: {e}" - ) + ); + ReparkOutcome::Failed } } } Ok(None) => { - tracing::warn!("loop: no session to re-park wake-up prompt for workspace {wid}") + tracing::warn!("loop: no session to re-park wake-up prompt for workspace {wid}"); + ReparkOutcome::NoSession } Err(e) => { - tracing::warn!("loop: failed to re-park wake-up prompt for workspace {wid}: {e}") + tracing::warn!("loop: failed to re-park wake-up prompt for workspace {wid}: {e}"); + ReparkOutcome::Failed } } } From 050ed84f65c7477a965c121ebdd6e643b551bd63 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 23:09:26 +0000 Subject: [PATCH 10/14] fix(cli-mode): drop the racing-attach paste-recovery to preserve never-lose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The codex backstop found the paste-recovery added a prompt-LOSS window: when a losing racing first-attach won 'new-session -A' with a promptless bootstrap, the recovery pasted the prompt into whatever agent owned the pane — but the loser's '--continue || fresh' leg and the eventual fresh agent share a process name, so the paste could land on the dying '--continue' leg yet be 'confirmed' by the fresh one, CAS-clearing a prompt no agent ever received. Revert to leaving the prompt parked when baked confirmation fails: in that rare double-first-attach race the next attach delivers it as a follow-up paste into the live agent — slower, but never lost, which is the invariant that matters. confirm_baked_prompt_consumed documents the deliberate choice. --- crates/server/src/routes/terminal.rs | 91 ++++++---------------------- 1 file changed, 19 insertions(+), 72 deletions(-) diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index fc859a025c..009c88ee13 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -17,10 +17,7 @@ use db::models::{ use deployment::Deployment; use executors::{ actions::ExecutorActionType, - executors::{ - BaseCodingAgent, StandardCodingAgentExecutor, - cli::{CliLaunchSpec, CliPromptArg}, - }, + executors::{BaseCodingAgent, StandardCodingAgentExecutor, cli::CliLaunchSpec}, profile::{ExecutorConfig, ExecutorConfigs}, }; use local_deployment::pty::{ @@ -364,26 +361,12 @@ async fn terminal_ws( } } CliPromptRouting::Baked(prompt) => { - // Argv agents (Positional/Flag) `rm` the staged file - // before exec, so a file left on disk after - // confirmation fails proves the bootstrap never ran and - // the prompt can be safely pasted as a recovery. - // StdinPipe carries no fallback (a lingering file can - // mean `cat` is still streaming — pasting would - // double-deliver). - let baked_paste_fallback = match spec.prompt_arg { - CliPromptArg::StdinPipe | CliPromptArg::Unsupported => None, - CliPromptArg::Positional | CliPromptArg::Flag(_) => { - Some(prompt.clone()) - } - }; baked_prompt = Some(prompt); prompt_delivery = Some(PromptDelivery { workspace_id: query.workspace_id, clear_session_id, peeked, deferred: None, - baked_paste_fallback, program: spec.program.clone(), claim, }); @@ -398,7 +381,6 @@ async fn terminal_ws( clear_session_id, peeked, deferred: Some(prompt), - baked_paste_fallback: None, program: spec.program.clone(), claim, }); @@ -472,16 +454,6 @@ struct PromptDelivery { /// Prompt to paste once the agent owns the pane (`None` = baked into the /// launch's temp-file transport instead). deferred: Option, - /// For a BAKED delivery only: the trimmed prompt to paste as a recovery - /// fallback if the baked bootstrap turns out never to have run (a racing - /// attach won `new-session -A` and created the pane with a promptless - /// launch, so our `-A` attach's baked bootstrap was ignored). `Some` only - /// for argv agents (`Positional`/`Flag`), whose bootstrap `rm`s the file - /// BEFORE exec — so a file still on disk proves the prompt was never - /// argv-delivered and pasting can't double-deliver. `None` for `StdinPipe` - /// (a lingering file there can mean `cat` is mid-stream) and for deferred - /// deliveries (which paste anyway). - baked_paste_fallback: Option, /// The agent binary this launch runs — delivery is only ever confirmed /// against THIS process owning the pane, so a fallback shell (or anything /// the user runs inside it) can never satisfy the gate. @@ -540,14 +512,7 @@ async fn handle_terminal_ws( Some(text) => { deliver_deferred_prompt(workspace_id, text, &delivery.program).await } - None => { - confirm_or_recover_baked_prompt( - workspace_id, - &delivery.program, - delivery.baked_paste_fallback.as_deref(), - ) - .await - } + None => confirm_baked_prompt_consumed(workspace_id, &delivery.program).await, }; if delivered { match Session::clear_pending_cli_prompt( @@ -678,32 +643,23 @@ async fn wait_for_cli_session(workspace_id: Uuid) -> bool { false } -/// Confirm a baked prompt reached the agent, recovering the racing-attach case. -/// -/// Normal confirmation: the staged file is gone (the bootstrap `rm`s it at -/// hand-off — see [`cli_prompt_file_exists`]) AND the expected agent owns the -/// pane. The agent-running condition catches the consumed-but-never-executed -/// window (`command -v` passed but the agent's exec failed: bad shebang, loader -/// error, permissions) where file-gone alone would clear a prompt no agent ever -/// received. +/// Confirm a baked prompt was actually handed to the agent: the staged file is +/// gone (the bootstrap `rm`s it at hand-off — see [`cli_prompt_file_exists`]) +/// AND the expected agent process owns the pane. The second condition catches +/// the consumed-but-never-executed window (`command -v` passed but the agent's +/// exec failed: bad shebang, loader error, permissions) where file-gone alone +/// would clear a prompt no agent ever received. An unconfirmed launch leaves +/// the parked DB prompt for the next attach's paste/fresh-launch retry — and +/// drops the never-consumed file so a later launch can't half-consume it. /// -/// Recovery: if confirmation times out but the file is STILL on disk and -/// `paste_fallback` is set (argv agents `rm` the file before exec, so a -/// surviving file proves our baked bootstrap never ran — typically because a -/// racing attach won `new-session -A` and created the pane with a promptless -/// launch, leaving our `-A` attach's bootstrap ignored), paste the prompt into -/// whatever agent now owns the pane instead of stranding it for a future -/// attach. Pasting is safe precisely because the file survived: the prompt was -/// never argv-delivered, so it can't double-deliver. -/// -/// Either way the never-consumed file is dropped so a later launch can't -/// half-consume it; an unconfirmed, unrecovered launch leaves the parked DB -/// prompt for the next attach's retry. -async fn confirm_or_recover_baked_prompt( - workspace_id: Uuid, - program: &str, - paste_fallback: Option<&str>, -) -> bool { +/// Note: a prompt stranded by a losing racing first-attach that won +/// `new-session -A` with a promptless bootstrap is deliberately NOT recovered +/// by pasting here — the loser's `--continue || fresh` leg and the eventual +/// fresh agent share a process name, so a paste could land on the dying +/// `--continue` leg yet be "confirmed" by the fresh one, silently losing the +/// prompt. Leaving it parked means the next attach delivers it as a follow-up +/// paste into the live agent: slower in that rare race, but never lost. +async fn confirm_baked_prompt_consumed(workspace_id: Uuid, program: &str) -> bool { for _ in 0..30 { if !cli_prompt_file_exists(workspace_id) && cli_pane_agent_running(workspace_id, program).await == Some(true) @@ -712,17 +668,8 @@ async fn confirm_or_recover_baked_prompt( } tokio::time::sleep(std::time::Duration::from_millis(250)).await; } - // Confirmation exhausted. A surviving file + an argv agent means the baked - // bootstrap never ran (a racing attach created the session first): paste - // the prompt into the live agent as a recovery. - let recovered = match paste_fallback { - Some(text) if cli_prompt_file_exists(workspace_id) => { - deliver_deferred_prompt(workspace_id, text, program).await - } - _ => false, - }; remove_cli_prompt_file(workspace_id); - recovered + false } /// Deliver a deferred prompt by paste once THE AGENT owns the pane. Pasting From c10841ed023aa4559ba3cf58fe7428c7e61a9a8f Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 23:09:26 +0000 Subject: [PATCH 11/14] fix(cli-mode): parse ps comm as the full remainder in the pane subtree walk pane_subtree_has_program split each ps line on whitespace and took the third token as comm, truncating a comm that contains spaces. macOS 'ps -o comm=' reports a full executable path, which can contain spaces (e.g. '/Applications/My App/codex'), so an agent there would never match and its delivery would never confirm. Split off only pid and ppid and keep the whole remainder as comm (normalize_comm still reduces it to the basename). --- crates/local-deployment/src/pty.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index d0851a573b..ed64c3a369 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -1004,15 +1004,22 @@ fn pane_subtree_has_program(ps_listing: &str, root_pid: u32, program: &str) -> b let mut comm_by_pid: HashMap = HashMap::new(); let mut children: HashMap> = HashMap::new(); for line in ps_listing.lines() { - let mut fields = line.split_whitespace(); - let (Some(pid), Some(ppid), Some(comm)) = (fields.next(), fields.next(), fields.next()) - else { + // `ps` right-justifies pid/ppid; split off exactly those two fields and + // keep the WHOLE remainder as comm — a comm can contain spaces (macOS + // `comm=` reports a full path, e.g. `/Applications/My App/codex`), and + // splitting it at the first space would strand a space-containing agent + // path (normalize_comm reduces it to the basename for the match). + let rest = line.trim_start(); + let Some((pid, rest)) = rest.split_once(char::is_whitespace) else { + continue; + }; + let Some((ppid, comm)) = rest.trim_start().split_once(char::is_whitespace) else { continue; }; let (Ok(pid), Ok(ppid)) = (pid.parse::(), ppid.parse::()) else { continue; }; - comm_by_pid.insert(pid, comm); + comm_by_pid.insert(pid, comm.trim()); children.entry(ppid).or_default().push(pid); } // Depth-first from the pane root, inclusive. `visited` guards against a @@ -2199,6 +2206,12 @@ mod tests { let ps_native = " 100 1 sh\n 200 100 claude\n"; assert!(pane_subtree_has_program(ps_native, 100, "claude")); + // macOS `ps -o comm=` reports a full path that can contain spaces; the + // whole remainder is the comm and normalize_comm reduces it to the + // basename, so a space in the path must not truncate the match. + let ps_macos = " 100 1 sh\n 200 100 /Applications/My App/codex\n"; + assert!(pane_subtree_has_program(ps_macos, 100, "codex")); + // A shell-only subtree (bootstrap still starting / missing-binary // fallback) must NOT satisfy the gate... assert!(!pane_subtree_has_program(" 100 1 sh\n", 100, "codex")); From f04c888a680ea68af252fef5521b75b0bb64493f Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 23:09:26 +0000 Subject: [PATCH 12/14] fix(loop-automation): keep manual wake-ups pending when a prompt is already parked The workspace-scoped re-park guard skipped parking whenever any prompt was already parked and the callers then marked the wake-up fired. For a limit-kind continuation that self-heals (the loop re-detects its banner), but a MANUAL wake-up is the user's explicit one-time prompt and was silently dropped, with no second slot to hold it. repark_prompt now returns AlreadyParked distinctly and wakeup_stays_pending keeps manual wake-ups pending (retry when the slot frees) while still letting limit continuations fire. --- .../local-deployment/src/loop_supervisor.rs | 82 ++++++++++++++----- 1 file changed, 60 insertions(+), 22 deletions(-) diff --git a/crates/local-deployment/src/loop_supervisor.rs b/crates/local-deployment/src/loop_supervisor.rs index 4941aa73ab..14ee2d1e4f 100644 --- a/crates/local-deployment/src/loop_supervisor.rs +++ b/crates/local-deployment/src/loop_supervisor.rs @@ -416,17 +416,19 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), // The tmux session may have been reaped before a long usage-window wake // came due — re-park the prompt so the next attach delivers it. if !cli_tmux_session_exists(wid).await { - if repark_prompt(pool, wid, &prompt).await == ReparkOutcome::Failed { - // Transient DB error: leave the wake-up pending so the next - // tick retries rather than dropping it (marking it fired here - // would neither deliver nor park the prompt). + let outcome = repark_prompt(pool, wid, &prompt).await; + if wakeup_stays_pending(&outcome, is_limit) { + // Not re-parked and not deliverable now (transient DB error, or + // a manual wake-up blocked behind an already-parked prompt): + // leave it pending so a later tick delivers it, rather than + // marking it fired and dropping the user's prompt. tracing::warn!( - "loop: workspace {wid} session gone; re-park failed transiently, \ - leaving wake-up pending" + "loop: workspace {wid} session gone; wake-up not re-parked ({outcome:?}), \ + leaving pending" ); continue; } - tracing::info!("loop: workspace {wid} session gone; re-parked wake-up prompt"); + tracing::info!("loop: workspace {wid} session gone; re-park outcome {outcome:?}"); ScheduledWakeup::mark_fired(pool, wakeup.id).await?; continue; } @@ -479,12 +481,13 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), // prompt so the next terminal attach delivers it rather than dropping // the wake-up — mirrors the session-gone branch above. tracing::warn!("loop: failed to deliver wake-up to workspace {wid}; re-parking"); - if repark_prompt(pool, wid, &prompt).await == ReparkOutcome::Failed { - // Transient DB error: leave the wake-up pending for the next - // tick instead of marking it fired and dropping it. + let outcome = repark_prompt(pool, wid, &prompt).await; + if wakeup_stays_pending(&outcome, is_limit) { + // Transient DB error, or a manual wake-up blocked behind an + // already-parked prompt: leave it pending for a later tick + // instead of marking it fired and dropping it. tracing::warn!( - "loop: re-park failed transiently for workspace {wid}; \ - leaving wake-up pending" + "loop: wake-up for workspace {wid} not re-parked ({outcome:?}); leaving pending" ); continue; } @@ -494,14 +497,18 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), Ok(()) } -/// Outcome of a [`repark_prompt`] attempt. Only `Failed` (a transient DB error) -/// warrants leaving the wake-up pending for a retry; every other outcome is -/// final and the caller may mark the wake-up fired. +/// Outcome of a [`repark_prompt`] attempt, so callers can decide whether to +/// mark the wake-up fired or leave it pending for a retry. #[derive(Debug, PartialEq, Eq)] enum ReparkOutcome { - /// The prompt is parked (or a prompt is already parked for the workspace, - /// which delivers first) — handled, mark the wake-up fired. - Handled, + /// This wake-up's prompt is now parked for the next attach to deliver. + Parked, + /// The workspace already had an undelivered prompt parked, so this wake-up's + /// prompt was NOT parked (the parked one delivers first). Fine to drop a + /// limit-kind continuation (the loop re-detects its banner afterwards), but + /// a MANUAL wake-up is the user's explicit prompt and must not be dropped — + /// the caller leaves it pending until the slot frees. + AlreadyParked, /// No session row exists to park on — nothing more the loop can do. NoSession, /// A transient DB error — leave the wake-up pending so the next tick retries @@ -530,7 +537,7 @@ async fn repark_prompt(pool: &sqlx::SqlitePool, wid: Uuid, prompt: &str) -> Repa tracing::info!( "loop: workspace {wid} already has a parked prompt; wake-up not re-parked" ); - return ReparkOutcome::Handled; + return ReparkOutcome::AlreadyParked; } Ok(None) => {} Err(e) => { @@ -541,9 +548,10 @@ async fn repark_prompt(pool: &sqlx::SqlitePool, wid: Uuid, prompt: &str) -> Repa match Session::find_latest_by_workspace_id(pool, wid).await { Ok(Some(session)) => { match Session::set_pending_cli_prompt_if_empty(pool, session.id, prompt).await { - // `false` = a prompt was parked between our check and here - // (raced an attach/another wake-up); either way one is queued. - Ok(_) => ReparkOutcome::Handled, + Ok(true) => ReparkOutcome::Parked, + // A prompt was parked between our check and here (raced an + // attach/another wake-up); treat it like the already-parked case. + Ok(false) => ReparkOutcome::AlreadyParked, Err(e) => { tracing::warn!( "loop: failed to re-park wake-up prompt for workspace {wid}: {e}" @@ -563,6 +571,20 @@ async fn repark_prompt(pool: &sqlx::SqlitePool, wid: Uuid, prompt: &str) -> Repa } } +/// Whether a wake-up whose re-park had this `outcome` must be left PENDING (not +/// marked fired). A transient DB error always retries. `AlreadyParked` drops a +/// limit-kind continuation (self-heals — the loop re-detects its banner) but +/// keeps a MANUAL wake-up pending, since a manual wake-up is the user's explicit +/// prompt and there is no second parking slot to hold it while the existing +/// prompt delivers. +fn wakeup_stays_pending(outcome: &ReparkOutcome, is_limit: bool) -> bool { + match outcome { + ReparkOutcome::Failed => true, + ReparkOutcome::AlreadyParked => !is_limit, + ReparkOutcome::Parked | ReparkOutcome::NoSession => false, + } +} + /// Scan each enabled workspace's pane and schedule a wake-up on a fresh limit. async fn detect_and_schedule(db: &DBService, now: DateTime) -> Result<(), sqlx::Error> { let pool = &db.pool; @@ -630,6 +652,22 @@ mod tests { parse_reset_at(text, now, utc()) } + #[test] + fn wakeup_pending_policy_keeps_manual_but_drops_limit_when_already_parked() { + // Transient DB error: always retry, regardless of kind. + assert!(wakeup_stays_pending(&ReparkOutcome::Failed, true)); + assert!(wakeup_stays_pending(&ReparkOutcome::Failed, false)); + // Already-parked: a limit continuation self-heals (re-detected), so + // drop it; a manual wake-up is the user's prompt, so keep it pending. + assert!(!wakeup_stays_pending(&ReparkOutcome::AlreadyParked, true)); + assert!(wakeup_stays_pending(&ReparkOutcome::AlreadyParked, false)); + // Parked / no-session are final for both kinds. + assert!(!wakeup_stays_pending(&ReparkOutcome::Parked, true)); + assert!(!wakeup_stays_pending(&ReparkOutcome::Parked, false)); + assert!(!wakeup_stays_pending(&ReparkOutcome::NoSession, true)); + assert!(!wakeup_stays_pending(&ReparkOutcome::NoSession, false)); + } + #[test] fn rate_limit_banner_is_classified_first() { let pane = "API Error: Server is temporarily limiting requests (not your usage limit) · Rate limited"; From e6040b2575a790f2e2e423ae6b35a4b03237628e Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 23:21:59 +0000 Subject: [PATCH 13/14] fix(cli-mode): close two residual delivery-ordering gaps found by the backstop - fresh_launch now uses the bootstrap's exact resume predicate (Uuid::parse_str == is_uuid). A non-UUID resume id is not resumed by the launch, so treating it as 'resuming' (the old resume_session_id.is_none() check) routed the prompt as a follow-up paste into continue_launch's doomed '--continue' leg, risking loss. - The loop supervisor's live-send path now peeks the workspace's parked prompt before send_cli_keys and defers the wake-up if one is queued, so a wake-up can never reach the agent ahead of an older undelivered prompt (the same 'parked prompt delivers first' invariant the re-park path enforces). --- .../local-deployment/src/loop_supervisor.rs | 20 +++++++++++++++++++ crates/server/src/routes/terminal.rs | 13 ++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/crates/local-deployment/src/loop_supervisor.rs b/crates/local-deployment/src/loop_supervisor.rs index 14ee2d1e4f..f0e097c9d8 100644 --- a/crates/local-deployment/src/loop_supervisor.rs +++ b/crates/local-deployment/src/loop_supervisor.rs @@ -468,6 +468,26 @@ async fn deliver_due_wakeups(db: &DBService, _now: DateTime) -> Result<(), continue; }; + // "Parked prompt delivers first" holds on the SEND path too: if the + // workspace has an undelivered prompt queued (an earlier delivery went + // unconfirmed and left it parked), don't send the wake-up ahead of it — + // that would reach the agent out of order. Leave the wake-up pending; + // a terminal attach delivers and clears the parked prompt, then a later + // tick delivers the wake-up. (Transient DB error: also retry.) + match Session::peek_pending_cli_prompt_for_workspace(pool, wid).await { + Ok(Some(_)) => { + tracing::debug!( + "loop: workspace {wid} has a parked prompt; deferring wake-up until it delivers" + ); + continue; + } + Ok(None) => {} + Err(e) => { + tracing::warn!("loop: failed to check parked prompt for workspace {wid}: {e}"); + continue; + } + } + if send_cli_keys(wid, &prompt).await { if is_limit { let _ = LoopAutomation::increment_attempts(pool, wid).await; diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 009c88ee13..3c95974e54 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -336,8 +336,17 @@ async fn terminal_ws( let mut baked_prompt = None; let mut deferred_prompt_pending = false; if let Some((peeked, claim, clear_session_id)) = carried { - let fresh_launch = resume_session_id.is_none() - && !cli_tmux_session_exists(query.workspace_id).await; + // Match the bootstrap's resume predicate EXACTLY (it filters the + // id through is_uuid before resuming): a non-UUID resume id is + // NOT resumed by the launch, so it must count as a fresh launch + // here too. Otherwise the prompt would route as a follow-up + // paste into a continue_launch's doomed `--continue` leg (the + // same loss hazard the racing-attach path avoids). + let resume_will_apply = resume_session_id + .as_deref() + .is_some_and(|id| Uuid::parse_str(id).is_ok()); + let fresh_launch = + !resume_will_apply && !cli_tmux_session_exists(query.workspace_id).await; let routed = if fresh_launch { route_initial_prompt(Some(peeked.clone()), &spec.prompt_arg) } else { From 7a8033d3863eb696d71ef434c144b35641ba35b6 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 23:41:37 +0000 Subject: [PATCH 14/14] fix(cli-mode): guard deferred paste against dying sessions and leading-dash text Two more delivery-loss paths found by the final backstop sweep: - send_cli_keys' small-text path ran 'tmux send-keys -l ' with no '--', so a prompt starting with '-' (e.g. '-rf ...') was parsed as an unknown send-keys flag and delivery silently failed (verified: 'unknown flag -f'). Add '--' before the text so it is always treated as a literal key. - deferred_prompt_pending was gated on fresh_launch (session absent at check time). A live session can exit between that check and 'new-session -A'; the freshly created pane would then run continue_launch's doomed '--continue' leg and the deferred paste could land on it and be lost. Request the bare-TUI bootstrap for any non-resume deferred prompt (ignored by '-A' when the session survives, so it's harmless in the common case). --- crates/local-deployment/src/pty.rs | 4 ++++ crates/server/src/routes/terminal.rs | 13 +++++++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index a7991d4943..048fcf9a29 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -1187,6 +1187,9 @@ pub async fn send_cli_keys(workspace_id: Uuid, text: &str) -> bool { let delivered = if needs_paste_transport(text) { paste_via_tmux_buffer(workspace_id, &target, text).await } else { + // `--` ends option parsing so text starting with `-` (e.g. a prompt + // like "-rf ...") is treated as a literal key rather than an unknown + // send-keys flag (verified: `send-keys -l "-x"` fails "unknown flag"). tmux_ok(&[ "-L", CLI_TMUX_SOCKET, @@ -1194,6 +1197,7 @@ pub async fn send_cli_keys(workspace_id: Uuid, text: &str) -> bool { "-t", &target, "-l", + "--", text, ]) .await diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index ab7a2b5591..a8f8e9b02d 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -398,10 +398,15 @@ async fn terminal_ws( }); } CliPromptRouting::Deferred(prompt) => { - // Only a FRESH launch needs the bare-TUI bootstrap; - // for an existing session (or resume) the bootstrap - // is ignored / resume wins. - deferred_prompt_pending = fresh_launch; + // Request the bare-TUI bootstrap whenever a resume won't + // apply — not just when the session is currently absent. + // A live session can exit between this check and + // `new-session -A`; if it does, the freshly created pane + // must NOT run `continue_launch`'s doomed `--continue` + // leg (the deferred paste could land on it and be lost). + // When the session survives, the bootstrap is ignored by + // `-A`, so requesting it is harmless. + deferred_prompt_pending = !resume_will_apply; prompt_delivery = Some(PromptDelivery { workspace_id: query.workspace_id, clear_session_id,