From 96b6419965a960d69c81b612be00862642c784e1 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 19 May 2026 17:22:08 +1000 Subject: [PATCH 1/7] feat(staged): cache interactive-login-shell env per project for pipeline steps Spawn `$SHELL -ils` once per working directory, capture its environment via `env -0` to a tempfile, and cache the snapshot for an hour. `run_pipeline_command` now applies the cached env to a native `sh -c` subprocess, so Hermit-managed PATH and other `.zshrc`-driven env vars are visible to pipeline steps (and any git hooks they trigger) without paying the per-call shell-init cost. Concurrent first-callers for the same working directory share a single capture via a `watch` channel. Falls back to the old `sh -lc` behaviour if capture fails. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_runner.rs | 39 ++- crates/acp-client/Cargo.toml | 2 +- crates/acp-client/src/lib.rs | 2 + crates/acp-client/src/shell_env.rs | 315 ++++++++++++++++++++ 4 files changed, 354 insertions(+), 4 deletions(-) create mode 100644 crates/acp-client/src/shell_env.rs diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 6ad9ffb5d..e9140e36c 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -37,7 +37,7 @@ use std::collections::HashMap; use std::io; use std::path::PathBuf; use std::process::{Command, Output, Stdio}; -use std::sync::Arc; +use std::sync::{Arc, OnceLock}; use std::time::Duration; use serde::{Deserialize, Serialize}; @@ -45,7 +45,7 @@ use tauri::AppHandle; use tokio::io::{AsyncRead, AsyncReadExt}; use tokio_util::sync::CancellationToken; -use acp_client::{McpServer, McpServerHttp}; +use acp_client::{McpServer, McpServerHttp, ShellEnvCache}; use crate::actions::{ActionExecutor, ActionRegistry}; use crate::agent::{AcpDriver, AgentDriver, MessageWriter}; @@ -1282,19 +1282,52 @@ async fn run_pipeline( PipelineOutcome::CompletedWithoutAi } +/// Shared cache of interactive-login-shell env snapshots, keyed by working +/// directory. Spawning `$SHELL -ils` to capture `.zshrc`-driven PATH (e.g. +/// Hermit) on every pipeline step costs ~50–500 ms; this amortises it to +/// once per project per TTL window. +pub fn shell_env_cache() -> &'static Arc { + static CACHE: OnceLock> = OnceLock::new(); + CACHE.get_or_init(|| Arc::new(ShellEnvCache::new())) +} + async fn run_pipeline_command( command: &str, working_dir: &PathBuf, cancel_token: &CancellationToken, ) -> io::Result { + // Apply the cached interactive-login-shell env so Hermit-managed + // binaries are on PATH (matters for git hooks invoked by pipeline + // steps). On capture failure fall back to `sh -lc`, which at least + // sources `/etc/profile`/`~/.profile`. + let snapshot = match shell_env_cache().get(working_dir).await { + Ok(env) => Some(env), + Err(e) => { + log::warn!( + "Failed to capture shell env for {}: {e}; falling back to sh -lc", + working_dir.display() + ); + None + } + }; + let mut cmd = tokio::process::Command::new("sh"); - cmd.args(["-lc", command]) + let sh_args: &[&str] = if snapshot.is_some() { + &["-c", command] + } else { + &["-lc", command] + }; + cmd.args(sh_args) .current_dir(working_dir) .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .kill_on_drop(true); + if let Some(snapshot) = &snapshot { + snapshot.apply_to(&mut cmd); + } + #[cfg(unix)] cmd.process_group(0); diff --git a/crates/acp-client/Cargo.toml b/crates/acp-client/Cargo.toml index 7c426c62d..a23b152e2 100644 --- a/crates/acp-client/Cargo.toml +++ b/crates/acp-client/Cargo.toml @@ -6,7 +6,7 @@ description = "Full-featured client for ACP (Agent Client Protocol) agents like [dependencies] agent-client-protocol = { version = "0.10", features = ["unstable"] } -tokio = { version = "1", features = ["process", "io-util", "time", "sync", "rt", "macros"] } +tokio = { version = "1", features = ["process", "io-util", "time", "sync", "rt", "macros", "fs"] } tokio-util = { version = "0.7", features = ["compat"] } serde = { version = "1", features = ["derive"] } serde_json = "1" diff --git a/crates/acp-client/src/lib.rs b/crates/acp-client/src/lib.rs index be16747a0..eb56676da 100644 --- a/crates/acp-client/src/lib.rs +++ b/crates/acp-client/src/lib.rs @@ -18,6 +18,7 @@ //! without session management mod driver; +mod shell_env; mod simple; mod types; @@ -29,6 +30,7 @@ pub use agent_client_protocol::{ pub use driver::{ strip_code_fences, AcpDriver, AgentDriver, BasicMessageWriter, MessageWriter, Store, }; +pub use shell_env::{ShellEnv, ShellEnvCache, DEFAULT_TTL as SHELL_ENV_DEFAULT_TTL}; pub use simple::run_acp_prompt; pub use types::{ discover_providers, find_acp_agent, find_acp_agent_by_id, find_command, known_agent_commands, diff --git a/crates/acp-client/src/shell_env.rs b/crates/acp-client/src/shell_env.rs new file mode 100644 index 000000000..2fc4c37c1 --- /dev/null +++ b/crates/acp-client/src/shell_env.rs @@ -0,0 +1,315 @@ +//! Per-working-directory snapshot cache of an interactive login shell's +//! environment. +//! +//! Spawns one `$SHELL -ils` per working directory (so directory-based PATH +//! managers like Hermit activate during `chpwd`/`precmd`), captures the +//! resulting environment via `env -0` redirected to a tempfile, and caches +//! the result for a TTL. Subsequent callers apply the snapshot to a native +//! [`tokio::process::Command`] via [`ShellEnv::apply_to`] — paying ~zero +//! per-call cost and producing no shell-init banners on stdout. +//! +//! Concurrent first-callers for the same working directory are coalesced +//! through a `watch` channel so only one shell is spawned per (dir, miss). + +use std::collections::HashMap; +use std::io; +use std::path::{Path, PathBuf}; +use std::process::Stdio; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; + +use tokio::io::AsyncWriteExt; +use tokio::process::Command; +use tokio::sync::watch; + +/// Default cache TTL — long enough that pipeline runs amortise the capture +/// cost, short enough that edits to `~/.zshrc` or `bin/activate-hermit` are +/// picked up within an hour without an explicit invalidation. +pub const DEFAULT_TTL: Duration = Duration::from_secs(60 * 60); + +/// Captured environment from a single interactive login shell invocation. +#[derive(Clone, Debug)] +pub struct ShellEnv { + vars: Arc>, + captured_at: Instant, +} + +impl ShellEnv { + /// Time when the snapshot was captured. + pub fn captured_at(&self) -> Instant { + self.captured_at + } + + /// Captured `KEY=VALUE` pairs. + pub fn vars(&self) -> &[(String, String)] { + &self.vars + } + + /// Clear `cmd`'s environment and replace it with the captured variables. + /// + /// Callers should set `current_dir`, args, and any per-call `extra_env` + /// overrides *after* `apply_to` so they win. + pub fn apply_to(&self, cmd: &mut Command) { + cmd.env_clear(); + for (k, v) in self.vars.iter() { + cmd.env(k, v); + } + } +} + +#[derive(Clone)] +enum CachedEntry { + Ready(ShellEnv), + InFlight(watch::Receiver>>), +} + +/// Cache of [`ShellEnv`] snapshots keyed by working directory. +pub struct ShellEnvCache { + inner: Mutex>, + ttl: Duration, +} + +impl ShellEnvCache { + /// Construct a cache with the default 1h TTL. + pub fn new() -> Self { + Self::with_ttl(DEFAULT_TTL) + } + + pub fn with_ttl(ttl: Duration) -> Self { + Self { + inner: Mutex::new(HashMap::new()), + ttl, + } + } + + /// Return a fresh-or-recent snapshot for `working_dir`. Spawns a shell on + /// miss/expiry; concurrent misses for the same dir share one capture. + pub async fn get(&self, working_dir: &Path) -> io::Result { + let key = working_dir.to_path_buf(); + + loop { + enum Action { + Wait(watch::Receiver>>), + Capture(watch::Sender>>), + } + + let action = { + let mut map = self.inner.lock().unwrap(); + match map.get(&key) { + Some(CachedEntry::Ready(env)) if env.captured_at.elapsed() < self.ttl => { + return Ok(env.clone()); + } + Some(CachedEntry::InFlight(rx)) => Action::Wait(rx.clone()), + _ => { + let (tx, rx) = watch::channel(None); + map.insert(key.clone(), CachedEntry::InFlight(rx)); + Action::Capture(tx) + } + } + }; + + match action { + Action::Wait(mut rx) => { + while rx.borrow().is_none() { + if rx.changed().await.is_err() { + // Sender dropped without delivering — re-check. + break; + } + } + if let Some(result) = rx.borrow().clone() { + return result.map_err(io::Error::other); + } + // Fall through to retry. + } + Action::Capture(tx) => { + let outcome = capture_shell_env(&key).await; + match outcome { + Ok(vars) => { + let env = ShellEnv { + vars: Arc::new(vars), + captured_at: Instant::now(), + }; + self.inner + .lock() + .unwrap() + .insert(key.clone(), CachedEntry::Ready(env.clone())); + let _ = tx.send(Some(Ok(env.clone()))); + return Ok(env); + } + Err(err) => { + let msg = err.to_string(); + { + let mut map = self.inner.lock().unwrap(); + if matches!(map.get(&key), Some(CachedEntry::InFlight(_))) { + map.remove(&key); + } + } + let _ = tx.send(Some(Err(msg))); + return Err(err); + } + } + } + } + } + } + + /// Drop the cached snapshot for `working_dir` (next `get` will recapture). + pub fn invalidate(&self, working_dir: &Path) { + self.inner.lock().unwrap().remove(working_dir); + } + + /// Drop all cached snapshots. + pub fn invalidate_all(&self) { + self.inner.lock().unwrap().clear(); + } +} + +impl Default for ShellEnvCache { + fn default() -> Self { + Self::new() + } +} + +async fn capture_shell_env(working_dir: &Path) -> io::Result> { + let shell = std::env::var("SHELL").unwrap_or_else(|_| "/bin/bash".to_string()); + + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or_default(); + let pid = std::process::id(); + let dump_path = std::env::temp_dir().join(format!("acp-shell-env-{pid}-{nanos}")); + let dump_path_str = dump_path.to_string_lossy().into_owned(); + + // Dump the environment NUL-delimited to a tempfile so values with + // newlines round-trip and shell-init banners on stdout are ignored + // entirely. + let script = format!( + "env -0 > {} 2>/dev/null\nexit\n", + single_quote(&dump_path_str) + ); + + let mut cmd = Command::new(&shell); + cmd.current_dir(working_dir) + .env_clear() + .env("HOME", std::env::var("HOME").unwrap_or_default()) + .env("USER", std::env::var("USER").unwrap_or_default()) + .env("SHELL", &shell) + .arg("-i") + .arg("-l") + .arg("-s") + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .kill_on_drop(true); + + let mut child = cmd.spawn()?; + { + let mut stdin = child + .stdin + .take() + .ok_or_else(|| io::Error::other("Failed to open shell stdin for env capture"))?; + stdin.write_all(script.as_bytes()).await?; + stdin.flush().await?; + // Drop stdin → shell sees EOF after `exit` and terminates cleanly. + } + + let status = child.wait().await?; + if !status.success() { + let _ = tokio::fs::remove_file(&dump_path).await; + return Err(io::Error::other(format!( + "Shell env capture exited with status {status}" + ))); + } + + let bytes = match tokio::fs::read(&dump_path).await { + Ok(b) => b, + Err(e) => { + let _ = tokio::fs::remove_file(&dump_path).await; + return Err(e); + } + }; + let _ = tokio::fs::remove_file(&dump_path).await; + + let mut vars = Vec::new(); + for chunk in bytes.split(|&b| b == 0) { + if chunk.is_empty() { + continue; + } + let Ok(s) = std::str::from_utf8(chunk) else { + continue; + }; + if let Some(eq_pos) = s.find('=') { + vars.push((s[..eq_pos].to_string(), s[eq_pos + 1..].to_string())); + } + } + Ok(vars) +} + +fn single_quote(value: &str) -> String { + let escaped = value.replace('\'', "'\\''"); + format!("'{escaped}'") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn captures_path_for_a_real_dir() { + let cache = ShellEnvCache::new(); + let env = cache + .get(&std::env::temp_dir()) + .await + .expect("snapshot should succeed"); + assert!( + env.vars().iter().any(|(k, _)| k == "PATH"), + "captured env should contain PATH" + ); + } + + #[tokio::test] + async fn returns_cached_value_within_ttl() { + let cache = ShellEnvCache::new(); + let dir = std::env::temp_dir(); + let first = cache.get(&dir).await.expect("first capture"); + let second = cache.get(&dir).await.expect("second capture"); + // Same Arc target → cache hit (the second call should not recapture). + assert_eq!(first.captured_at(), second.captured_at()); + } + + #[tokio::test] + async fn invalidate_forces_recapture() { + let cache = ShellEnvCache::new(); + let dir = std::env::temp_dir(); + let first = cache.get(&dir).await.expect("first capture"); + cache.invalidate(&dir); + let second = cache.get(&dir).await.expect("second capture"); + assert!(second.captured_at() >= first.captured_at()); + assert_ne!(first.captured_at(), second.captured_at()); + } + + #[tokio::test] + async fn apply_to_replaces_env() { + let cache = ShellEnvCache::new(); + let env = cache + .get(&std::env::temp_dir()) + .await + .expect("snapshot should succeed"); + let mut cmd = Command::new("/usr/bin/env"); + env.apply_to(&mut cmd); + // Sanity: env_clear was called, so apply_to fully owns the resulting env. + // We can't directly observe env on tokio::process::Command, but we can + // run it and confirm the output contains a captured variable. + let output = cmd.output().await.expect("env should run"); + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("PATH=")); + } + + #[test] + fn single_quote_escapes_quotes() { + assert_eq!(single_quote("plain"), "'plain'"); + assert_eq!(single_quote("with 'quote'"), "'with '\\''quote'\\'''"); + } +} From 68f3fef78cfc5edec0a312d367ed986e81f31aa0 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 19 May 2026 17:57:54 +1000 Subject: [PATCH 2/7] refactor(staged): move shell_env from acp-client to src-tauri MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-cwd interactive-login-shell env capture is general-purpose pipeline plumbing, not ACP-shaped: `apps/staged/src-tauri/src/session_runner.rs` is the sole consumer and `acp-client` itself never calls `ShellEnvCache`. Colocate the module with its only caller and drop the cross-crate re-export. No behaviour change. Tempfile prefix renamed `acp-shell-env-*` → `staged-shell-env-*`. Adds the `fs` feature to `src-tauri`'s tokio features for `tokio::fs::read`/`remove_file`. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/Cargo.toml | 2 +- apps/staged/src-tauri/src/lib.rs | 1 + apps/staged/src-tauri/src/session_runner.rs | 3 ++- {crates/acp-client => apps/staged/src-tauri}/src/shell_env.rs | 2 +- crates/acp-client/src/lib.rs | 2 -- 5 files changed, 5 insertions(+), 5 deletions(-) rename {crates/acp-client => apps/staged/src-tauri}/src/shell_env.rs (99%) diff --git a/apps/staged/src-tauri/Cargo.toml b/apps/staged/src-tauri/Cargo.toml index 40d359abb..7a5346015 100644 --- a/apps/staged/src-tauri/Cargo.toml +++ b/apps/staged/src-tauri/Cargo.toml @@ -40,7 +40,7 @@ dirs = "6.0" tauri-plugin-clipboard-manager = "2.3.2" tauri-plugin-window-state = "2.4.1" reqwest = { version = "0.13.1", features = ["json"] } -tokio = { version = "1.50.0", features = ["sync", "process", "io-util", "macros", "rt-multi-thread", "time"] } +tokio = { version = "1.50.0", features = ["sync", "process", "io-util", "macros", "rt-multi-thread", "time", "fs"] } tauri-plugin-opener = "2" tauri-plugin-dialog = "2" tauri-plugin-process = "2" diff --git a/apps/staged/src-tauri/src/lib.rs b/apps/staged/src-tauri/src/lib.rs index fadfb299b..2ac377a7f 100644 --- a/apps/staged/src-tauri/src/lib.rs +++ b/apps/staged/src-tauri/src/lib.rs @@ -21,6 +21,7 @@ pub mod prs; pub mod review_commands; pub mod session_commands; pub mod session_runner; +pub mod shell_env; pub mod store; pub(crate) mod terminal_output; pub mod timeline; diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index e9140e36c..285148b49 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -45,11 +45,12 @@ use tauri::AppHandle; use tokio::io::{AsyncRead, AsyncReadExt}; use tokio_util::sync::CancellationToken; -use acp_client::{McpServer, McpServerHttp, ShellEnvCache}; +use acp_client::{McpServer, McpServerHttp}; use crate::actions::{ActionExecutor, ActionRegistry}; use crate::agent::{AcpDriver, AgentDriver, MessageWriter}; use crate::git::Span; +use crate::shell_env::ShellEnvCache; use crate::store::{ Comment, CommentAuthor, CommentType, CompletionReason, FailureStrategy, MessageRole, PipelineExecution, PipelineKind, PipelineStep, SessionStatus, StepStatus, StepType, Store, diff --git a/crates/acp-client/src/shell_env.rs b/apps/staged/src-tauri/src/shell_env.rs similarity index 99% rename from crates/acp-client/src/shell_env.rs rename to apps/staged/src-tauri/src/shell_env.rs index 2fc4c37c1..1bb063500 100644 --- a/crates/acp-client/src/shell_env.rs +++ b/apps/staged/src-tauri/src/shell_env.rs @@ -178,7 +178,7 @@ async fn capture_shell_env(working_dir: &Path) -> io::Result Date: Wed, 20 May 2026 10:26:29 +1000 Subject: [PATCH 3/7] chore(staged): drop unused tokio fs feature from acp-client `shell_env` moved to `src-tauri` in 68f3fef7, taking the only consumers of `tokio::fs::read`/`remove_file` with it. Drop the now-orphaned `fs` feature so `acp-client`'s tokio features match what it actually uses. No behaviour change. Signed-off-by: Matt Toohey --- crates/acp-client/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/acp-client/Cargo.toml b/crates/acp-client/Cargo.toml index a23b152e2..7c426c62d 100644 --- a/crates/acp-client/Cargo.toml +++ b/crates/acp-client/Cargo.toml @@ -6,7 +6,7 @@ description = "Full-featured client for ACP (Agent Client Protocol) agents like [dependencies] agent-client-protocol = { version = "0.10", features = ["unstable"] } -tokio = { version = "1", features = ["process", "io-util", "time", "sync", "rt", "macros", "fs"] } +tokio = { version = "1", features = ["process", "io-util", "time", "sync", "rt", "macros"] } tokio-util = { version = "0.7", features = ["compat"] } serde = { version = "1", features = ["derive"] } serde_json = "1" From 9e6e4c82d63767a03c1315c5bf465f526ef739ab Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 20 May 2026 10:38:31 +1000 Subject: [PATCH 4/7] fix(staged): evict stale InFlight entries from shell env cache on cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ShellEnvCache::get` inserts a `CachedEntry::InFlight(rx)` into the map before awaiting `capture_shell_env`, then promotes it to `Ready` (or removes it) in the `Ok`/`Err` arms. Neither arm runs when the future is dropped mid-await — and `run_pipeline_command` awaits `get` inside a cancellable pipeline step, so cancellation is a real path. When that happened, `tx` dropped without sending, but the `InFlight(rx)` entry stayed in the map. Subsequent callers cloned the dead receiver, saw `rx.changed()` return `Err`, fell through with `rx.borrow()` still `None`, and retried the outer loop — finding the same dead entry. Hot spin until the cache was dropped. Bind the `InFlight` lifetime to a stack-allocated `InFlightGuard` whose `Drop` removes the entry unless it has been promoted to `Ready`. Declared inside the `Capture` arm body after the `tx` match binding, so on cancellation the guard drops first (evict) and `tx` drops second (signal waiters `Err`). Waiters wake, see no entry on retry, and become the next Capturer. Also handles a panic inside `capture_shell_env` for free, and lets the explicit `map.remove` in the `Err` arm go away. Regression test: spawn a capturer, `yield_now` so it inserts `InFlight` and parks inside `capture_shell_env`, abort it, then assert (a) the map has no `InFlight` for the dir and (b) a subsequent `get` completes within a bounded timeout instead of spinning. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/shell_env.rs | 75 +++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 6 deletions(-) diff --git a/apps/staged/src-tauri/src/shell_env.rs b/apps/staged/src-tauri/src/shell_env.rs index 1bb063500..0eecdc650 100644 --- a/apps/staged/src-tauri/src/shell_env.rs +++ b/apps/staged/src-tauri/src/shell_env.rs @@ -69,6 +69,30 @@ pub struct ShellEnvCache { ttl: Duration, } +/// Removes an `InFlight` entry from the cache if its capture future is +/// dropped (cancellation or panic) before publishing a `Ready` result. +/// +/// Without this, a cancelled capture would leave a stale `InFlight(rx)` in +/// the map whose `tx` is gone; subsequent callers would clone the receiver, +/// wake immediately on `Err`, and spin the outer retry loop forever. +struct InFlightGuard<'a> { + cache: &'a ShellEnvCache, + key: &'a Path, + promoted: bool, +} + +impl Drop for InFlightGuard<'_> { + fn drop(&mut self) { + if self.promoted { + return; + } + let mut map = self.cache.inner.lock().unwrap(); + if matches!(map.get(self.key), Some(CachedEntry::InFlight(_))) { + map.remove(self.key); + } + } +} + impl ShellEnvCache { /// Construct a cache with the default 1h TTL. pub fn new() -> Self { @@ -122,6 +146,15 @@ impl ShellEnvCache { // Fall through to retry. } Action::Capture(tx) => { + // Declared after `tx` (a match binding) so it drops first + // on cancellation/panic: evict the InFlight entry before + // `tx` drops and signals waiters Err. Waiters then retry, + // find no entry, and become the next Capturer. + let mut guard = InFlightGuard { + cache: self, + key: &key, + promoted: false, + }; let outcome = capture_shell_env(&key).await; match outcome { Ok(vars) => { @@ -133,17 +166,12 @@ impl ShellEnvCache { .lock() .unwrap() .insert(key.clone(), CachedEntry::Ready(env.clone())); + guard.promoted = true; let _ = tx.send(Some(Ok(env.clone()))); return Ok(env); } Err(err) => { let msg = err.to_string(); - { - let mut map = self.inner.lock().unwrap(); - if matches!(map.get(&key), Some(CachedEntry::InFlight(_))) { - map.remove(&key); - } - } let _ = tx.send(Some(Err(msg))); return Err(err); } @@ -312,4 +340,39 @@ mod tests { assert_eq!(single_quote("plain"), "'plain'"); assert_eq!(single_quote("with 'quote'"), "'with '\\''quote'\\'''"); } + + #[tokio::test] + async fn cancelled_capture_evicts_stale_inflight() { + let cache = Arc::new(ShellEnvCache::new()); + let dir = std::env::temp_dir(); + + // Drive a Capturer just long enough to insert InFlight, then abort it. + // On a current_thread runtime, yield_now hands control to the spawned + // task; it inserts InFlight before its first internal `.await`, then + // parks somewhere inside `capture_shell_env`. + let first = tokio::spawn({ + let cache = cache.clone(); + let dir = dir.clone(); + async move { cache.get(&dir).await } + }); + tokio::task::yield_now().await; + first.abort(); + let _ = first.await; + + // The guard's Drop must have evicted the InFlight entry — otherwise + // subsequent callers would clone a dead receiver and spin. + { + let map = cache.inner.lock().unwrap(); + assert!( + !matches!(map.get(&dir), Some(CachedEntry::InFlight(_))), + "InFlight entry leaked after capture future was cancelled" + ); + } + + // And a subsequent caller must complete (not spin) within a sane bound. + let second = tokio::time::timeout(Duration::from_secs(30), cache.get(&dir)) + .await + .expect("second caller must not spin"); + assert!(second.is_ok()); + } } From 0e9ad295733fed259f077a267dfa56ea080de4c4 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 20 May 2026 12:29:15 +1000 Subject: [PATCH 5/7] feat(staged): route git/cli::run through cached shell env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `git/cli::run` is the central choke point for every `cli::run` callsite in `apps/staged/src-tauri/src/git/*` — commits, worktree adds/removes, checkouts, fetches, fast-forward merges, cherry-picks, clones via the `git clone` fallback path in `github.rs`. All of those previously inherited the Tauri process's PATH, which on macOS launched from Finder/Dock is `/usr/bin:/bin:/usr/sbin:/sbin` — so Hermit-shimmed git, LFS filters, credential helpers, and any binary invoked by a fired hook (pre-commit, commit-msg, post-commit, post-checkout, post-merge) would silently miss anything `~/.zshrc` would have put on PATH. Add `ShellEnvCache::get_blocking`, a synchronous mirror of `get` that reuses the same per-cwd `Ready` snapshot map. On a fresh miss it spawns `$SHELL -ils` synchronously and stores the result; it does not engage the `InFlight` watch-channel coalescing — a sync caller that races an async caller will do its own capture and overwrite, which is harmless because both shells produce equivalent env. `cli::run` now applies the snapshot via the new `ShellEnv::apply_to_std`, falling back to the prior `strip_git_env`-on-inherited-env behaviour if capture fails. The integration is gated `cfg(not(test))` because each unit test runs in a fresh tempdir, so the cache always misses and would add hundreds of ms per test for zero test value. Production builds always take the cache path. Factor `dump_path`/`dump_script`/`parse_env_dump` out of `capture_shell_env` so the new `capture_shell_env_blocking` shares the same tempfile naming, env-dump script, and parser — keeping the two capture paths in lockstep. Tempfile prefix unchanged. Also lands two testability seams that were already staged in the working tree from the test plan: `ShellEnvCache::with_shell_and_ttl` (point the cache at a hermetic fake shell) and `run_pipeline_command_with_cache` (inject a pre-seeded cache instead of the global singleton). No production behaviour change from either. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/git/cli.rs | 33 ++- apps/staged/src-tauri/src/session_runner.rs | 13 +- apps/staged/src-tauri/src/shell_env.rs | 219 +++++++++++++++++--- 3 files changed, 239 insertions(+), 26 deletions(-) diff --git a/apps/staged/src-tauri/src/git/cli.rs b/apps/staged/src-tauri/src/git/cli.rs index 761175e24..2cab285a5 100644 --- a/apps/staged/src-tauri/src/git/cli.rs +++ b/apps/staged/src-tauri/src/git/cli.rs @@ -30,7 +30,7 @@ pub fn run(repo: &Path, args: &[&str]) -> Result { let mut command = Command::new("git"); command.args(["-C", repo_str]).args(args); - strip_git_env(&mut command); + apply_shell_env(&mut command, repo); let output = command.output().map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { @@ -50,3 +50,34 @@ pub fn run(repo: &Path, args: &[&str]) -> Result { String::from_utf8(output.stdout).map_err(|_| GitError::InvalidUtf8) } + +/// Replace the spawned git's environment with the project's cached +/// interactive-login-shell snapshot so Hermit-managed `git`, LFS filters, +/// credential helpers, and any binaries invoked by git hooks see the same +/// PATH/env that a user's terminal sees. +/// +/// On capture failure (e.g. `$SHELL` unset, init script exits non-zero), +/// falls back to the parent process env with `GIT_*` variables stripped — +/// matching the pre-cache behaviour. +/// +/// Gated behind `cfg(not(test))` because unit tests run in fresh tempdirs +/// where shell init has no project context and the per-test spawn would +/// add ~hundreds of ms of overhead with no test value. +#[cfg(not(test))] +fn apply_shell_env(command: &mut Command, repo: &Path) { + match crate::session_runner::shell_env_cache().get_blocking(repo) { + Ok(env) => env.apply_to_std(command), + Err(e) => { + log::warn!( + "Failed to capture shell env for {}: {e}; falling back to inherited env", + repo.display() + ); + strip_git_env(command); + } + } +} + +#[cfg(test)] +fn apply_shell_env(command: &mut Command, _repo: &Path) { + strip_git_env(command); +} diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 285148b49..552c27eab 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -1296,12 +1296,23 @@ async fn run_pipeline_command( command: &str, working_dir: &PathBuf, cancel_token: &CancellationToken, +) -> io::Result { + run_pipeline_command_with_cache(shell_env_cache(), command, working_dir, cancel_token).await +} + +/// Same as [`run_pipeline_command`] but lets the caller pass an explicit cache. +/// Used by tests to pre-seed snapshots or point at a hermetic fake `$SHELL`. +async fn run_pipeline_command_with_cache( + cache: &ShellEnvCache, + command: &str, + working_dir: &PathBuf, + cancel_token: &CancellationToken, ) -> io::Result { // Apply the cached interactive-login-shell env so Hermit-managed // binaries are on PATH (matters for git hooks invoked by pipeline // steps). On capture failure fall back to `sh -lc`, which at least // sources `/etc/profile`/`~/.profile`. - let snapshot = match shell_env_cache().get(working_dir).await { + let snapshot = match cache.get(working_dir).await { Ok(env) => Some(env), Err(e) => { log::warn!( diff --git a/apps/staged/src-tauri/src/shell_env.rs b/apps/staged/src-tauri/src/shell_env.rs index 0eecdc650..ad6f578e7 100644 --- a/apps/staged/src-tauri/src/shell_env.rs +++ b/apps/staged/src-tauri/src/shell_env.rs @@ -55,6 +55,15 @@ impl ShellEnv { cmd.env(k, v); } } + + /// Std-process variant of [`apply_to`] for synchronous callers (notably + /// `git/cli.rs::run`). + pub fn apply_to_std(&self, cmd: &mut std::process::Command) { + cmd.env_clear(); + for (k, v) in self.vars.iter() { + cmd.env(k, v); + } + } } #[derive(Clone)] @@ -67,6 +76,7 @@ enum CachedEntry { pub struct ShellEnvCache { inner: Mutex>, ttl: Duration, + shell: PathBuf, } /// Removes an `InFlight` entry from the cache if its capture future is @@ -100,9 +110,20 @@ impl ShellEnvCache { } pub fn with_ttl(ttl: Duration) -> Self { + Self::with_shell_and_ttl(resolve_shell(), ttl) + } + + /// Construct a cache that spawns `shell` instead of `$SHELL`. Tests use this + /// to point at a hermetic script; production code should use [`new`] or + /// [`with_ttl`]. + /// + /// [`new`]: ShellEnvCache::new + /// [`with_ttl`]: ShellEnvCache::with_ttl + pub fn with_shell_and_ttl(shell: PathBuf, ttl: Duration) -> Self { Self { inner: Mutex::new(HashMap::new()), ttl, + shell, } } @@ -155,7 +176,7 @@ impl ShellEnvCache { key: &key, promoted: false, }; - let outcome = capture_shell_env(&key).await; + let outcome = capture_shell_env(&key, &self.shell).await; match outcome { Ok(vars) => { let env = ShellEnv { @@ -181,6 +202,41 @@ impl ShellEnvCache { } } + /// Synchronous variant of [`get`] for sync callers like `git/cli.rs::run`. + /// + /// Returns a `Ready` snapshot if one is present and fresh; otherwise + /// spawns a *blocking* `$SHELL -ils` capture and stores the result. + /// + /// Does **not** coordinate with `InFlight` async captures — if a sync + /// caller races an async caller for the same dir, both will capture + /// independently and the second writer wins. Semantically safe (both + /// captures produce equivalent env); only cost is duplicate shell-init + /// work in that narrow first-call window. + /// + /// [`get`]: ShellEnvCache::get + pub fn get_blocking(&self, working_dir: &Path) -> io::Result { + let key = working_dir.to_path_buf(); + { + let map = self.inner.lock().unwrap(); + if let Some(CachedEntry::Ready(env)) = map.get(&key) { + if env.captured_at.elapsed() < self.ttl { + return Ok(env.clone()); + } + } + } + + let vars = capture_shell_env_blocking(&key, &self.shell)?; + let env = ShellEnv { + vars: Arc::new(vars), + captured_at: Instant::now(), + }; + self.inner + .lock() + .unwrap() + .insert(key, CachedEntry::Ready(env.clone())); + Ok(env) + } + /// Drop the cached snapshot for `working_dir` (next `get` will recapture). pub fn invalidate(&self, working_dir: &Path) { self.inner.lock().unwrap().remove(working_dir); @@ -198,31 +254,62 @@ impl Default for ShellEnvCache { } } -async fn capture_shell_env(working_dir: &Path) -> io::Result> { - let shell = std::env::var("SHELL").unwrap_or_else(|_| "/bin/bash".to_string()); +/// Resolve the shell binary the cache should spawn. Reads `$SHELL`, falling +/// back to `/bin/bash` — matching the canonical interactive-login-shell +/// wrapper used elsewhere in the codebase. +fn resolve_shell() -> PathBuf { + std::env::var_os("SHELL") + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from("/bin/bash")) +} +/// Allocate a unique tempfile path for the env dump. +fn dump_path() -> PathBuf { let nanos = SystemTime::now() .duration_since(UNIX_EPOCH) .map(|d| d.as_nanos()) .unwrap_or_default(); let pid = std::process::id(); - let dump_path = std::env::temp_dir().join(format!("staged-shell-env-{pid}-{nanos}")); - let dump_path_str = dump_path.to_string_lossy().into_owned(); + std::env::temp_dir().join(format!("staged-shell-env-{pid}-{nanos}")) +} - // Dump the environment NUL-delimited to a tempfile so values with - // newlines round-trip and shell-init banners on stdout are ignored - // entirely. - let script = format!( +/// Build the shell script that dumps the interactive-login env NUL-delimited +/// to `dump_path` and exits. The tempfile path is single-quoted so values +/// with newlines round-trip and shell-init banners on stdout are ignored. +fn dump_script(dump_path: &Path) -> String { + let dump_path_str = dump_path.to_string_lossy(); + format!( "env -0 > {} 2>/dev/null\nexit\n", single_quote(&dump_path_str) - ); + ) +} + +fn parse_env_dump(bytes: &[u8]) -> Vec<(String, String)> { + let mut vars = Vec::new(); + for chunk in bytes.split(|&b| b == 0) { + if chunk.is_empty() { + continue; + } + let Ok(s) = std::str::from_utf8(chunk) else { + continue; + }; + if let Some(eq_pos) = s.find('=') { + vars.push((s[..eq_pos].to_string(), s[eq_pos + 1..].to_string())); + } + } + vars +} - let mut cmd = Command::new(&shell); +async fn capture_shell_env(working_dir: &Path, shell: &Path) -> io::Result> { + let dump_path = dump_path(); + let script = dump_script(&dump_path); + + let mut cmd = Command::new(shell); cmd.current_dir(working_dir) .env_clear() .env("HOME", std::env::var("HOME").unwrap_or_default()) .env("USER", std::env::var("USER").unwrap_or_default()) - .env("SHELL", &shell) + .env("SHELL", shell) .arg("-i") .arg("-l") .arg("-s") @@ -259,19 +346,64 @@ async fn capture_shell_env(working_dir: &Path) -> io::Result io::Result> { + use std::io::Write as _; + + let dump_path = dump_path(); + let script = dump_script(&dump_path); + + let mut cmd = std::process::Command::new(shell); + cmd.current_dir(working_dir) + .env_clear() + .env("HOME", std::env::var("HOME").unwrap_or_default()) + .env("USER", std::env::var("USER").unwrap_or_default()) + .env("SHELL", shell) + .arg("-i") + .arg("-l") + .arg("-s") + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()); + + let mut child = cmd.spawn()?; + { + let mut stdin = child + .stdin + .take() + .ok_or_else(|| io::Error::other("Failed to open shell stdin for env capture"))?; + stdin.write_all(script.as_bytes())?; + stdin.flush()?; + // Drop stdin → shell sees EOF after `exit` and terminates cleanly. + } + + let status = child.wait()?; + if !status.success() { + let _ = std::fs::remove_file(&dump_path); + return Err(io::Error::other(format!( + "Shell env capture exited with status {status}" + ))); } - Ok(vars) + + let bytes = match std::fs::read(&dump_path) { + Ok(b) => b, + Err(e) => { + let _ = std::fs::remove_file(&dump_path); + return Err(e); + } + }; + let _ = std::fs::remove_file(&dump_path); + + Ok(parse_env_dump(&bytes)) } fn single_quote(value: &str) -> String { @@ -341,6 +473,45 @@ mod tests { assert_eq!(single_quote("with 'quote'"), "'with '\\''quote'\\'''"); } + #[test] + fn get_blocking_captures_and_caches() { + let cache = ShellEnvCache::new(); + let dir = std::env::temp_dir(); + + let first = cache + .get_blocking(&dir) + .expect("blocking snapshot should succeed"); + assert!( + first.vars().iter().any(|(k, _)| k == "PATH"), + "captured env should contain PATH" + ); + + let second = cache + .get_blocking(&dir) + .expect("second blocking snapshot should hit cache"); + assert_eq!( + first.captured_at(), + second.captured_at(), + "second blocking call should return cached snapshot" + ); + } + + #[tokio::test] + async fn get_blocking_sees_snapshots_from_async_get() { + let cache = Arc::new(ShellEnvCache::new()); + let dir = std::env::temp_dir(); + + let async_env = cache.get(&dir).await.expect("async capture should succeed"); + let sync_env = cache + .get_blocking(&dir) + .expect("blocking call should hit cache populated by async path"); + assert_eq!( + async_env.captured_at(), + sync_env.captured_at(), + "sync caller should observe the async-populated snapshot" + ); + } + #[tokio::test] async fn cancelled_capture_evicts_stale_inflight() { let cache = Arc::new(ShellEnvCache::new()); From df38ac2959b6061ab0873a62661daf794bfc61c5 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 20 May 2026 14:23:42 +1000 Subject: [PATCH 6/7] test(staged): expand shell-env cache + pipeline integration test suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the four-wave test plan for the per-cwd `ShellEnvCache` and the `run_pipeline_command_with_cache` integration seam landed in 0e9ad295. The existing tests covered the happy path and the cancellation regression from 9e6e4c82, but left concurrency, TTL expiry, failure modes, fallback behavior, and tempfile cleanup unverified. Wave 1 (pure unit, no fake shell): - `ttl_expiry_triggers_recapture` — exercises the stale-`Ready` branch - `invalidate_all_clears_every_entry` / `invalidate_only_affects_target_dir` - `different_dirs_cached_independently` - `apply_to_clears_existing_env` — locks in `env_clear` against a future drop, complementing the existing PATH-is-present assertion - `apply_to_then_env_lets_caller_override` — docstring contract on post-`apply_to` overrides - `single_quote_empty_input` / `single_quote_backslash_unchanged` / `single_quote_pure_quote` Wave 2 (single capture): - `working_dir_argument_is_honoured` — `PWD` matches `working_dir` - `home_and_user_passed_through` — guards the `env_clear` whitelist - `promoted_guard_keeps_ready_entry` — locks `promoted = true` semantics Wave 3 (concurrency + failure modes; uses the `with_shell_and_ttl` seam): - `concurrent_first_callers_share_one_capture` — all callers see the same `captured_at` (watch-channel coalescing) - `waiter_wakes_when_capturer_cancels` — bounded-timeout assertion that a Waiter doesn't stall when the Capturer is aborted - `capture_err_propagates_and_is_not_cached` — both concurrent waiters see `Err` and a third call recaptures (no negative caching) - `tempfile_cleaned_up_on_success` / `tempfile_cleaned_up_on_failure` — set-difference snapshot of `staged-shell-env-*` so parallel tests don't interfere - `values_with_newlines_round_trip` — locks in NUL-framing over line-based parsing Wave 4 (`run_pipeline_command_with_cache` integration): - `snapshot_path_cached_env_reaches_child` — child sees a token baked into the cached snapshot - `fallback_path_when_cache_returns_err` — falling back to `sh -lc` when capture fails still runs the command - `cancellation_under_snapshot_branch` — `kill_on_drop` + select arm still cancel under the snapshot path (the existing test mostly hits the fallback branch) - `current_dir_survives_apply_to` — `pwd` reports the requested dir Pre-warm the global cache in `pipeline_command_cancellation_stops_current_step` so its 4-second deadline measures pure cancellation latency rather than the first-time shell-init cost, which can blow the budget under the heavier parallel-test load this PR adds. All wave-3/4 fake-shell tests are `#[cfg(unix)]` (PermissionsExt) and use a tempfile-backed script so they don't depend on the developer's `$SHELL` or `.zshrc`. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/session_runner.rs | 163 +++++++ apps/staged/src-tauri/src/shell_env.rs | 474 ++++++++++++++++++++ 2 files changed, 637 insertions(+) diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 552c27eab..c28a09175 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -2389,6 +2389,14 @@ mod tests { #[cfg(unix)] #[tokio::test] async fn pipeline_command_cancellation_stops_current_step() { + // Pre-warm the global cache so the elapsed-time assertion measures pure + // cancellation latency rather than first-time shell-env capture (which + // can take seconds under parallel-test load). + let _ = shell_env_cache() + .get(&std::env::temp_dir()) + .await + .expect("warm cache"); + let cancel_token = CancellationToken::new(); let cancel_after_start = cancel_token.clone(); tokio::spawn(async move { @@ -2407,6 +2415,161 @@ mod tests { assert!(started.elapsed() < Duration::from_secs(4)); } + // --------------------------------------------------------------------- + // Integration: `run_pipeline_command_with_cache` snapshot/fallback paths + // + // These tests inject a hermetic `ShellEnvCache` via the test seam so + // pipeline behaviour can be exercised without depending on the + // developer's `$SHELL` or `.zshrc`. + // --------------------------------------------------------------------- + + /// Write `content` to a 0755 tempfile suitable for use as `$SHELL`. + #[cfg(unix)] + fn write_fake_shell(content: &str) -> tempfile::NamedTempFile { + use std::io::Write as _; + use std::os::unix::fs::PermissionsExt; + let mut file = tempfile::Builder::new() + .prefix("staged-fake-shell-") + .suffix(".sh") + .tempfile() + .expect("create fake shell tempfile"); + file.write_all(content.as_bytes()).expect("write script"); + file.flush().expect("flush script"); + let mut perms = std::fs::metadata(file.path()).unwrap().permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(file.path(), perms).expect("chmod 755"); + file + } + + /// G20: When the cache produces a snapshot, its env vars reach the child + /// process spawned for the pipeline step. + #[cfg(unix)] + #[tokio::test] + async fn snapshot_path_cached_env_reaches_child() { + let shell = write_fake_shell( + "#!/bin/sh\nPATH=/usr/bin:/bin\nPIPELINE_TEST_TOKEN=snapshot-marker-abc\nexport PATH PIPELINE_TEST_TOKEN\nexec /bin/sh -s\n", + ); + let cache = ShellEnvCache::with_shell_and_ttl( + shell.path().to_path_buf(), + Duration::from_secs(3600), + ); + let dir = tempfile::tempdir().expect("tempdir"); + + let cancel = CancellationToken::new(); + let result = run_pipeline_command_with_cache( + &cache, + "echo $PIPELINE_TEST_TOKEN", + &dir.path().to_path_buf(), + &cancel, + ) + .await + .expect("run_pipeline_command_with_cache should succeed"); + + let output = match result { + PipelineCommandResult::Completed(o) => o, + PipelineCommandResult::Cancelled { .. } => panic!("unexpected cancellation"), + }; + assert!(output.status.success(), "command should succeed"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("snapshot-marker-abc"), + "child must see PIPELINE_TEST_TOKEN from the snapshot; stdout={stdout:?}" + ); + } + + /// G21: When the cache returns `Err`, `run_pipeline_command_with_cache` + /// falls back to `sh -lc` and the command still runs. + #[cfg(unix)] + #[tokio::test] + async fn fallback_path_when_cache_returns_err() { + let shell = write_fake_shell("#!/bin/sh\nexit 1\n"); + let cache = ShellEnvCache::with_shell_and_ttl( + shell.path().to_path_buf(), + Duration::from_secs(3600), + ); + let dir = std::env::temp_dir(); + + let cancel = CancellationToken::new(); + let result = run_pipeline_command_with_cache(&cache, "echo fallback-ok", &dir, &cancel) + .await + .expect("fallback path should still spawn and run"); + + let output = match result { + PipelineCommandResult::Completed(o) => o, + PipelineCommandResult::Cancelled { .. } => panic!("unexpected cancellation"), + }; + assert!(output.status.success(), "fallback sh -lc should succeed"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("fallback-ok"), + "fallback should still produce the echo output; stdout={stdout:?}" + ); + } + + /// G22: Cancellation still terminates the child even after a snapshot is + /// applied — guards against a future refactor that loses `kill_on_drop` + /// or the cancellation `select!` arm. + #[cfg(unix)] + #[tokio::test] + async fn cancellation_under_snapshot_branch() { + let shell = + write_fake_shell("#!/bin/sh\nPATH=/usr/bin:/bin\nexport PATH\nexec /bin/sh -s\n"); + let cache = ShellEnvCache::with_shell_and_ttl( + shell.path().to_path_buf(), + Duration::from_secs(3600), + ); + let dir = std::env::temp_dir(); + + let cancel_token = CancellationToken::new(); + let cancel_after_start = cancel_token.clone(); + tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(100)).await; + cancel_after_start.cancel(); + }); + + let started = std::time::Instant::now(); + let result = + run_pipeline_command_with_cache(&cache, "sleep 5 & wait", &dir, &cancel_token).await; + assert!(matches!( + result, + Ok(PipelineCommandResult::Cancelled { .. }) + )); + assert!(started.elapsed() < Duration::from_secs(4)); + } + + /// G23: `current_dir` survives `apply_to` — `pwd` reports the directory + /// passed to `run_pipeline_command_with_cache`, not the test's cwd. + #[cfg(unix)] + #[tokio::test] + async fn current_dir_survives_apply_to() { + let shell = + write_fake_shell("#!/bin/sh\nPATH=/usr/bin:/bin\nexport PATH\nexec /bin/sh -s\n"); + let cache = ShellEnvCache::with_shell_and_ttl( + shell.path().to_path_buf(), + Duration::from_secs(3600), + ); + let dir = tempfile::tempdir().expect("tempdir"); + let resolved = std::fs::canonicalize(dir.path()).unwrap_or_else(|_| dir.path().to_owned()); + + let cancel = CancellationToken::new(); + let result = run_pipeline_command_with_cache(&cache, "pwd", &resolved, &cancel) + .await + .expect("pwd should succeed"); + let output = match result { + PipelineCommandResult::Completed(o) => o, + PipelineCommandResult::Cancelled { .. } => panic!("unexpected cancellation"), + }; + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + let reported = stdout.trim(); + let reported_path = std::fs::canonicalize(PathBuf::from(reported)) + .unwrap_or_else(|_| PathBuf::from(reported)); + assert_eq!( + reported_path, resolved, + "child should run in the requested working_dir; pwd reported {reported:?}" + ); + } + #[test] fn pipeline_command_output_collapses_progress_for_prompt() { let output = combine_normalized_command_output(b"10%\r20%\rdone\n", b""); diff --git a/apps/staged/src-tauri/src/shell_env.rs b/apps/staged/src-tauri/src/shell_env.rs index ad6f578e7..b6bc01077 100644 --- a/apps/staged/src-tauri/src/shell_env.rs +++ b/apps/staged/src-tauri/src/shell_env.rs @@ -414,6 +414,54 @@ fn single_quote(value: &str) -> String { #[cfg(test)] mod tests { use super::*; + use std::collections::HashSet; + + // --------------------------------------------------------------------- + // Test helpers + // --------------------------------------------------------------------- + + /// Write `content` to a 0755 tempfile suitable for use as `$SHELL`. + /// + /// Hold the returned handle for the test's lifetime — dropping it removes + /// the file. Unix-only because we set the executable mode. + #[cfg(unix)] + fn write_fake_shell(content: &str) -> tempfile::NamedTempFile { + use std::io::Write as _; + use std::os::unix::fs::PermissionsExt; + let mut file = tempfile::Builder::new() + .prefix("staged-fake-shell-") + .suffix(".sh") + .tempfile() + .expect("create fake shell tempfile"); + file.write_all(content.as_bytes()).expect("write script"); + file.flush().expect("flush script"); + let mut perms = std::fs::metadata(file.path()).unwrap().permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(file.path(), perms).expect("chmod 755"); + file + } + + /// Snapshot the set of `staged-shell-env-*` files currently in temp_dir(). + /// Used by tempfile-cleanup tests to assert no orphan was created across a + /// single capture (set difference, so parallel captures don't interfere). + fn snapshot_orphan_paths() -> HashSet { + std::fs::read_dir(std::env::temp_dir()) + .map(|rd| { + rd.flatten() + .filter(|e| { + e.file_name() + .to_string_lossy() + .starts_with("staged-shell-env-") + }) + .map(|e| e.path()) + .collect() + }) + .unwrap_or_default() + } + + // --------------------------------------------------------------------- + // Existing tests (preserved) + // --------------------------------------------------------------------- #[tokio::test] async fn captures_path_for_a_real_dir() { @@ -546,4 +594,430 @@ mod tests { .expect("second caller must not spin"); assert!(second.is_ok()); } + + // --------------------------------------------------------------------- + // Wave 1: Pure unit tests (no fake-shell, no concurrency) + // --------------------------------------------------------------------- + + /// A1: A `Ready` entry whose `captured_at` is older than `ttl` triggers a + /// fresh capture instead of being returned. Exercises the stale-`Ready` + /// branch the in-TTL test cannot reach. + #[tokio::test] + async fn ttl_expiry_triggers_recapture() { + let cache = ShellEnvCache::with_ttl(Duration::from_millis(50)); + let dir = std::env::temp_dir(); + let first = cache.get(&dir).await.expect("first capture"); + tokio::time::sleep(Duration::from_millis(75)).await; + let second = cache.get(&dir).await.expect("recapture after TTL"); + assert_ne!( + first.captured_at(), + second.captured_at(), + "TTL expiry must force a fresh capture" + ); + assert!(second.captured_at() > first.captured_at()); + } + + /// A2: `invalidate_all` empties the internal map and forces both dirs to + /// recapture on the next `get`. + #[tokio::test] + async fn invalidate_all_clears_every_entry() { + let cache = ShellEnvCache::new(); + let dir_a = std::env::temp_dir(); + let dir_b = tempfile::tempdir().expect("tempdir b"); + + let a_first = cache.get(&dir_a).await.expect("capture a"); + let b_first = cache.get(dir_b.path()).await.expect("capture b"); + + cache.invalidate_all(); + assert!( + cache.inner.lock().unwrap().is_empty(), + "invalidate_all should empty the map" + ); + + let a_second = cache.get(&dir_a).await.expect("recapture a"); + let b_second = cache.get(dir_b.path()).await.expect("recapture b"); + assert_ne!(a_first.captured_at(), a_second.captured_at()); + assert_ne!(b_first.captured_at(), b_second.captured_at()); + } + + /// A3: Two distinct working dirs are cached independently — each second + /// `get` hits its own `Ready` entry without recapture. + #[tokio::test] + async fn different_dirs_cached_independently() { + let cache = ShellEnvCache::new(); + let dir_a = std::env::temp_dir(); + let dir_b = tempfile::tempdir().expect("tempdir b"); + + let a_first = cache.get(&dir_a).await.expect("capture a"); + let b_first = cache.get(dir_b.path()).await.expect("capture b"); + // Per-dir cache hits. + let a_second = cache.get(&dir_a).await.expect("a hit"); + let b_second = cache.get(dir_b.path()).await.expect("b hit"); + assert_eq!(a_first.captured_at(), a_second.captured_at()); + assert_eq!(b_first.captured_at(), b_second.captured_at()); + // Distinct captures across dirs. + assert_ne!(a_first.captured_at(), b_first.captured_at()); + } + + /// A4: `invalidate(A)` must not disturb dir B's cached snapshot. + #[tokio::test] + async fn invalidate_only_affects_target_dir() { + let cache = ShellEnvCache::new(); + let dir_a = std::env::temp_dir(); + let dir_b = tempfile::tempdir().expect("tempdir b"); + let a_first = cache.get(&dir_a).await.expect("capture a"); + let b_first = cache.get(dir_b.path()).await.expect("capture b"); + + cache.invalidate(&dir_a); + + let a_second = cache.get(&dir_a).await.expect("recapture a"); + let b_second = cache + .get(dir_b.path()) + .await + .expect("b should still be cached"); + + assert_ne!( + a_first.captured_at(), + a_second.captured_at(), + "A should have been recaptured" + ); + assert_eq!( + b_first.captured_at(), + b_second.captured_at(), + "B's snapshot must be unaffected by invalidating A" + ); + } + + /// D13: `apply_to` must call `env_clear` — i.e. any env vars set on the + /// `Command` *before* `apply_to` must NOT leak into the child. + #[tokio::test] + async fn apply_to_clears_existing_env() { + let cache = ShellEnvCache::new(); + let env = cache + .get(&std::env::temp_dir()) + .await + .expect("snapshot should succeed"); + let mut cmd = Command::new("/usr/bin/env"); + cmd.env("PIPELINE_TEST_SHOULD_NOT_LEAK", "1"); + env.apply_to(&mut cmd); + let output = cmd.output().await.expect("env should run"); + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + !stdout.contains("PIPELINE_TEST_SHOULD_NOT_LEAK"), + "apply_to must clear pre-existing env vars (env_clear): stdout = {stdout}" + ); + } + + /// D14: Env vars set *after* `apply_to` win — locks in the docstring + /// contract so per-call `extra_env` overrides keep working. + #[tokio::test] + async fn apply_to_then_env_lets_caller_override() { + let cache = ShellEnvCache::new(); + let env = cache + .get(&std::env::temp_dir()) + .await + .expect("snapshot should succeed"); + let mut cmd = Command::new("/usr/bin/env"); + env.apply_to(&mut cmd); + cmd.env("PATH", "/sentinel"); + let output = cmd.output().await.expect("env should run"); + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("PATH=/sentinel"), + "post-apply_to env override must win: stdout = {stdout}" + ); + } + + /// F17: Empty input → empty single-quote pair. + #[test] + fn single_quote_empty_input() { + assert_eq!(single_quote(""), "''"); + } + + /// F18: Backslashes pass through unchanged — sh single-quotes don't + /// interpret backslashes. Locks in current behavior against a future + /// "harden the escape" PR that might over-escape. + #[test] + fn single_quote_backslash_unchanged() { + assert_eq!(single_quote(r"a\b"), r"'a\b'"); + } + + /// F19: Pure quote becomes the three-character close/escape/open dance. + #[test] + fn single_quote_pure_quote() { + assert_eq!(single_quote("'"), "''\\'''"); + } + + // --------------------------------------------------------------------- + // Wave 2: Single-capture mechanics (no concurrency, no fake shell) + // --------------------------------------------------------------------- + + /// C11: The `current_dir` is honoured by the spawned shell — `PWD` in the + /// captured env matches the directory we asked to capture in. + #[tokio::test] + async fn working_dir_argument_is_honoured() { + let dir = tempfile::tempdir().expect("tempdir"); + // Resolve symlinks so the `PWD` the shell reports lines up with what + // we expect (macOS's TMPDIR is under a symlink-laden path). + let resolved = std::fs::canonicalize(dir.path()).unwrap_or_else(|_| dir.path().to_owned()); + let cache = ShellEnvCache::new(); + let env = cache.get(&resolved).await.expect("capture should succeed"); + let pwd = env + .vars() + .iter() + .find(|(k, _)| k == "PWD") + .map(|(_, v)| v.clone()) + .expect("captured env should include PWD"); + let pwd_path = + std::fs::canonicalize(PathBuf::from(&pwd)).unwrap_or_else(|_| PathBuf::from(&pwd)); + assert_eq!( + pwd_path, resolved, + "PWD should match the working_dir argument" + ); + } + + /// C12: `HOME` and `USER` from the parent process are passed through into + /// the captured snapshot — guards against `env_clear` accidentally + /// dropping the whitelist. + #[tokio::test] + async fn home_and_user_passed_through() { + let cache = ShellEnvCache::new(); + let env = cache + .get(&std::env::temp_dir()) + .await + .expect("snapshot should succeed"); + let find = |k| { + env.vars() + .iter() + .find(|(n, _)| n == k) + .map(|(_, v)| v.clone()) + }; + + if let Ok(parent_home) = std::env::var("HOME") { + if !parent_home.is_empty() { + let captured = find("HOME").unwrap_or_default(); + assert_eq!( + captured, parent_home, + "HOME must round-trip into the snapshot" + ); + } + } + if let Ok(parent_user) = std::env::var("USER") { + if !parent_user.is_empty() { + let captured = find("USER").unwrap_or_default(); + assert_eq!( + captured, parent_user, + "USER must round-trip into the snapshot" + ); + } + } + } + + /// E15: A successful capture promotes the entry to `Ready` and leaves it + /// in the map (not removed, not still `InFlight`). Locks in the + /// `promoted = true` semantics. + #[tokio::test] + async fn promoted_guard_keeps_ready_entry() { + let cache = ShellEnvCache::new(); + let dir = std::env::temp_dir(); + let _ = cache.get(&dir).await.expect("capture"); + let map = cache.inner.lock().unwrap(); + match map.get(&dir) { + Some(CachedEntry::Ready(_)) => {} + other => panic!( + "expected Ready after successful capture, got {:?}", + other.is_some() + ), + } + } + + // --------------------------------------------------------------------- + // Wave 3: Concurrency + failure modes (needs fake-shell seam) + // --------------------------------------------------------------------- + + /// B5: Concurrent first-callers coalesce through the watch channel — only + /// one shell is spawned per (dir, miss) and every caller sees the same + /// `captured_at`. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn concurrent_first_callers_share_one_capture() { + let cache = Arc::new(ShellEnvCache::new()); + let dir = std::env::temp_dir(); + + let mut handles = Vec::new(); + for _ in 0..8 { + let cache = cache.clone(); + let dir = dir.clone(); + handles.push(tokio::spawn(async move { cache.get(&dir).await })); + } + let mut captured_ats = Vec::new(); + for h in handles { + let env = h.await.expect("task").expect("capture"); + captured_ats.push(env.captured_at()); + } + let first = captured_ats[0]; + assert!( + captured_ats.iter().all(|c| *c == first), + "all coalesced callers should observe the same captured_at: {:?}", + captured_ats + ); + } + + /// B6: When the active Capturer is aborted, a parallel Waiter must wake + /// (either becoming the next Capturer and succeeding, or seeing a + /// recapture-then-Ok) within a bounded timeout — no spin-loop. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn waiter_wakes_when_capturer_cancels() { + let cache = Arc::new(ShellEnvCache::new()); + let dir = tempfile::tempdir().expect("tempdir"); + let dir_path = dir.path().to_path_buf(); + + // First task becomes the Capturer; it inserts InFlight before its + // first internal `.await` inside `capture_shell_env`. + let capturer = tokio::spawn({ + let cache = cache.clone(); + let dir = dir_path.clone(); + async move { cache.get(&dir).await } + }); + // Give the capturer a chance to insert InFlight. + tokio::task::yield_now().await; + tokio::time::sleep(Duration::from_millis(10)).await; + + // Second task should arrive as a Waiter on the same InFlight rx. + let waiter = tokio::spawn({ + let cache = cache.clone(); + let dir = dir_path.clone(); + async move { tokio::time::timeout(Duration::from_secs(30), cache.get(&dir)).await } + }); + tokio::task::yield_now().await; + + capturer.abort(); + let _ = capturer.await; + + // Waiter must complete within a bounded time (not spin), and ultimately + // succeed because the cancellation evicts InFlight so it can recapture. + let waiter_result = waiter.await.expect("waiter task"); + let inner = waiter_result.expect("waiter must not spin past timeout"); + assert!( + inner.is_ok(), + "waiter should ultimately succeed after capturer cancellation: {inner:?}" + ); + } + + /// B7: A failing capture propagates `Err` to all concurrent waiters AND is + /// not negative-cached — a subsequent `get` must launch a fresh capture. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn capture_err_propagates_and_is_not_cached() { + let shell = write_fake_shell("#!/bin/sh\nexit 1\n"); + let cache = Arc::new(ShellEnvCache::with_shell_and_ttl( + shell.path().to_path_buf(), + Duration::from_secs(3600), + )); + let dir = tempfile::tempdir().expect("tempdir"); + let dir_path = dir.path().to_path_buf(); + + let h1 = tokio::spawn({ + let cache = cache.clone(); + let dir = dir_path.clone(); + async move { cache.get(&dir).await } + }); + let h2 = tokio::spawn({ + let cache = cache.clone(); + let dir = dir_path.clone(); + async move { cache.get(&dir).await } + }); + let (r1, r2) = (h1.await.expect("h1"), h2.await.expect("h2")); + assert!(r1.is_err(), "concurrent caller 1 should see Err: {r1:?}"); + assert!(r2.is_err(), "concurrent caller 2 should see Err: {r2:?}"); + + // No negative caching — failures must not persist. + { + let map = cache.inner.lock().unwrap(); + assert!( + map.get(&dir_path).is_none(), + "failed capture must not leave an entry in the cache" + ); + } + + // A third caller spawns a fresh capture (which also fails). + let r3 = cache.get(&dir_path).await; + assert!(r3.is_err(), "third caller should still see Err: {r3:?}"); + } + + /// C8: Tempfile is cleaned up after both successful and failing captures. + /// Uses set-difference vs. a pre-snapshot so parallel tests don't perturb. + #[cfg(unix)] + #[tokio::test] + async fn tempfile_cleaned_up_on_success() { + let shell = + write_fake_shell("#!/bin/sh\nPATH=/usr/bin:/bin\nexport PATH\nexec /bin/sh -s\n"); + let cache = ShellEnvCache::with_shell_and_ttl( + shell.path().to_path_buf(), + Duration::from_secs(3600), + ); + let dir = tempfile::tempdir().expect("tempdir"); + + let pre = snapshot_orphan_paths(); + let env = cache.get(dir.path()).await.expect("capture"); + assert!( + env.vars().iter().any(|(k, _)| k == "PATH"), + "fake shell should set PATH" + ); + let post = snapshot_orphan_paths(); + let new_files: Vec<&PathBuf> = post.difference(&pre).collect(); + assert!( + new_files.is_empty(), + "successful capture must not leak tempfiles: {new_files:?}" + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn tempfile_cleaned_up_on_failure() { + let shell = write_fake_shell("#!/bin/sh\nexit 1\n"); + let cache = ShellEnvCache::with_shell_and_ttl( + shell.path().to_path_buf(), + Duration::from_secs(3600), + ); + let dir = tempfile::tempdir().expect("tempdir"); + + let pre = snapshot_orphan_paths(); + let result = cache.get(dir.path()).await; + assert!(result.is_err(), "failing fake shell should produce Err"); + let post = snapshot_orphan_paths(); + let new_files: Vec<&PathBuf> = post.difference(&pre).collect(); + assert!( + new_files.is_empty(), + "failed capture must not leak tempfiles: {new_files:?}" + ); + } + + /// C9: Values containing newlines round-trip through the NUL-delimited + /// `env -0` dump — locks in the choice to use NUL framing instead of + /// line-based parsing. + #[cfg(unix)] + #[tokio::test] + async fn values_with_newlines_round_trip() { + let shell = write_fake_shell( + "#!/bin/sh\nPATH=/usr/bin:/bin\nWEIRD='line1\nline2'\nexport PATH WEIRD\nexec /bin/sh -s\n", + ); + let cache = ShellEnvCache::with_shell_and_ttl( + shell.path().to_path_buf(), + Duration::from_secs(3600), + ); + let dir = tempfile::tempdir().expect("tempdir"); + + let env = cache.get(dir.path()).await.expect("capture"); + let weird = env + .vars() + .iter() + .find(|(k, _)| k == "WEIRD") + .map(|(_, v)| v.clone()) + .expect("WEIRD must round-trip into the snapshot"); + assert_eq!(weird, "line1\nline2", "newlines must round-trip verbatim"); + } } From d643078c879fde37cb95430b8f71171d6defe7b2 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 20 May 2026 15:59:23 +1000 Subject: [PATCH 7/7] test(staged): tighten shell-env concurrency tests with fake-shell seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `with_shell_and_ttl` seam was introduced in 0e9ad295 specifically so concurrency tests could be deterministic, but `concurrent_first_callers_share_one_capture` and `waiter_wakes_when_capturer_cancels` still used `ShellEnvCache::new()` and the real `$SHELL`. On a fast capture (the common case on dev machines with a warm cache), call 0 finished before calls 1–7 arrived, so B5's "all `captured_at` equal" assertion passed via the cache-hit path without ever exercising the watch-channel coalescing branch. Point both tests at a fake shell whose `sleep 0.5` guarantees the Capturer parks inside `capture_shell_env` while the other 7 callers (B5) or the parallel Waiter (B6) arrive on the same `InFlight(rx)`. B6's recapture path becomes fast and deterministic too, so its post-abort timeout shrinks from 30s to 3s — wide enough for the expected ~600ms path (abort → guard evicts InFlight → waiter retries → new capture's `sleep 0.5` + parse) but tight enough to catch slower-but-not-infinite regressions, not just the hard hangs the cancellation fix originally targeted. Also drop `single_quote_pure_quote` (F19): the existing `single_quote_escapes_quotes` already covers the close/escape/open dance via the mid-string `'quote'` case, so F19 just costs a line. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/shell_env.rs | 43 +++++++++++++++++++------- 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/apps/staged/src-tauri/src/shell_env.rs b/apps/staged/src-tauri/src/shell_env.rs index b6bc01077..180fea3a6 100644 --- a/apps/staged/src-tauri/src/shell_env.rs +++ b/apps/staged/src-tauri/src/shell_env.rs @@ -744,12 +744,6 @@ mod tests { assert_eq!(single_quote(r"a\b"), r"'a\b'"); } - /// F19: Pure quote becomes the three-character close/escape/open dance. - #[test] - fn single_quote_pure_quote() { - assert_eq!(single_quote("'"), "''\\'''"); - } - // --------------------------------------------------------------------- // Wave 2: Single-capture mechanics (no concurrency, no fake shell) // --------------------------------------------------------------------- @@ -840,16 +834,28 @@ mod tests { /// B5: Concurrent first-callers coalesce through the watch channel — only /// one shell is spawned per (dir, miss) and every caller sees the same /// `captured_at`. + /// + /// The fake shell's leading `sleep 0.5` guarantees all 8 callers arrive + /// during `InFlight(rx)` rather than racing a sub-ms real-`$SHELL` capture + /// (which would still pass the `captured_at` assertion via the cache-hit + /// path, without proving watch-channel coalescing actually fired). #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn concurrent_first_callers_share_one_capture() { - let cache = Arc::new(ShellEnvCache::new()); - let dir = std::env::temp_dir(); + let shell = write_fake_shell( + "#!/bin/sh\nsleep 0.5\nPATH=/usr/bin:/bin\nexport PATH\nexec /bin/sh -s\n", + ); + let cache = Arc::new(ShellEnvCache::with_shell_and_ttl( + shell.path().to_path_buf(), + Duration::from_secs(3600), + )); + let dir = tempfile::tempdir().expect("tempdir"); + let dir_path = dir.path().to_path_buf(); let mut handles = Vec::new(); for _ in 0..8 { let cache = cache.clone(); - let dir = dir.clone(); + let dir = dir_path.clone(); handles.push(tokio::spawn(async move { cache.get(&dir).await })); } let mut captured_ats = Vec::new(); @@ -868,10 +874,23 @@ mod tests { /// B6: When the active Capturer is aborted, a parallel Waiter must wake /// (either becoming the next Capturer and succeeding, or seeing a /// recapture-then-Ok) within a bounded timeout — no spin-loop. + /// + /// Uses the fake-shell seam so the post-cancellation recapture is fast + /// and deterministic. The 3s budget is tight enough to catch + /// slower-but-not-infinite regressions (~600ms is the expected path: + /// abort → guard evicts InFlight → waiter retries → new capture's + /// `sleep 0.5` + parse), not just the hard hang from the original + /// cancellation bug. #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn waiter_wakes_when_capturer_cancels() { - let cache = Arc::new(ShellEnvCache::new()); + let shell = write_fake_shell( + "#!/bin/sh\nsleep 0.5\nPATH=/usr/bin:/bin\nexport PATH\nexec /bin/sh -s\n", + ); + let cache = Arc::new(ShellEnvCache::with_shell_and_ttl( + shell.path().to_path_buf(), + Duration::from_secs(3600), + )); let dir = tempfile::tempdir().expect("tempdir"); let dir_path = dir.path().to_path_buf(); @@ -884,13 +903,13 @@ mod tests { }); // Give the capturer a chance to insert InFlight. tokio::task::yield_now().await; - tokio::time::sleep(Duration::from_millis(10)).await; + tokio::time::sleep(Duration::from_millis(50)).await; // Second task should arrive as a Waiter on the same InFlight rx. let waiter = tokio::spawn({ let cache = cache.clone(); let dir = dir_path.clone(); - async move { tokio::time::timeout(Duration::from_secs(30), cache.get(&dir)).await } + async move { tokio::time::timeout(Duration::from_secs(3), cache.get(&dir)).await } }); tokio::task::yield_now().await;