From 26672712fbda0a65c4571bfad5cb00244eeff63c Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Thu, 10 Sep 2026 10:39:45 -0700 Subject: [PATCH 1/3] fix(agent): take the workspace-map walk off the turn's critical path Every turn assembled its context by walking the session working directory synchronously, on the tokio worker driving the turn, with no timeout, no cancellation point and no log line. The 2026-09-10 test drive of main measured what that costs: with the default working directory ($HOME on macOS) the walk blocked in std::fs::read_dir -> __opendir2 under ~/Library/Group Containers and the turn never reached the provider at all. 100% of /usr/bin/sample samples sat in that one stack across four captures; the daemon idled at 0.0% CPU while the composer said "Thinking" for 8m32s. Stop could not end it, and each wedged turn leaked its worker for the life of the process. Four rules, each pinned by a test: - No filesystem syscall on the async path. The walk, the root's stat and canonicalize all move into the blocking task. The cache is therefore keyed on the path as given and the fast path is TTL-only; the mtime early-invalidation it replaces cost a stat on the turn's path. - The walk runs under spawn_blocking with a finite budget (CONTEXT_WORKSPACE_SUMMARY_BUDGET_MS, 1500ms). On lapse the turn proceeds without a map, one WARN names the directory and the lever, and the negative is cached for the TTL. The budget cannot be set to zero. - The wait is tied to the turn's cancellation token, threaded down from reply_internal through assemble_turn_context, inject_moim and collect_moim, so Stop is honoured immediately. - Single-flight per root. A blocked std::fs call cannot be cancelled, so a wedged thread is lost; the marker bounds that to one thread per root rather than one per turn, and later turns are served the last good map. A home directory is also no longer walked at all, along with filesystem roots and the OS/cloud-sync trees (~/Library, ~/AppData, Group Containers, CloudStorage, Mobile Documents, FileProvider). Those trees are pruned mid-walk too, and the walk no longer crosses a mount point. Pre-existing: neither file changed in f350cdfc..c5392640. --- crates/biorouter/src/agents/agent.rs | 8 + .../biorouter/src/agents/extension_manager.rs | 10 +- crates/biorouter/src/agents/moim.rs | 8 +- .../biorouter/src/agents/workspace_summary.rs | 799 ++++++++++++++++-- docs/agent-loop/README.md | 1 + docs/agent-loop/workspace-map.md | 156 ++++ 6 files changed, 914 insertions(+), 68 deletions(-) create mode 100644 docs/agent-loop/workspace-map.md diff --git a/crates/biorouter/src/agents/agent.rs b/crates/biorouter/src/agents/agent.rs index 4d1fd9be7..6f033d14d 100644 --- a/crates/biorouter/src/agents/agent.rs +++ b/crates/biorouter/src/agents/agent.rs @@ -5447,11 +5447,17 @@ impl Agent { /// assistant messages). MOIM injection already re-normalized on the way /// through; this closes the same hole for sessions with no MOIM provider. /// `BIOROUTER_NORMALIZE_EACH_TURN=false` restores the old behaviour. + /// `cancel` is the turn's own token. It reaches this far down because the + /// workspace map's directory walk is the one piece of per-turn context + /// assembly that touches the filesystem, and a `Stop` has to be able to end + /// the wait for it — see `agents::workspace_summary` and finding M1 of the + /// 2026-09-10 test drive. async fn assemble_turn_context( &self, session_id: &str, conversation: &Conversation, working_dir: &std::path::Path, + cancel: Option<&CancellationToken>, ) -> Conversation { let _phase = super::phase_timing::Phase::start("agent.assemble_turn_context"); @@ -5462,6 +5468,7 @@ impl Agent { &self.extension_manager, working_dir, &self.normalizer, + cancel, ) .await; drop(moim_phase); @@ -9578,6 +9585,7 @@ impl Agent { &session_config.id, &conversation, &working_dir, + cancel_token.as_ref(), ) .await } diff --git a/crates/biorouter/src/agents/extension_manager.rs b/crates/biorouter/src/agents/extension_manager.rs index 936902cfc..bc59c2e69 100644 --- a/crates/biorouter/src/agents/extension_manager.rs +++ b/crates/biorouter/src/agents/extension_manager.rs @@ -3152,10 +3152,14 @@ impl ExtensionManager { .map(|ext| ext.get_client()) } + /// `cancel` is the turn's cancellation token, threaded down to the + /// workspace map so a `Stop` is honoured while its directory walk is still + /// outstanding. See `agents::workspace_summary`. pub async fn collect_moim( &self, session_id: &str, working_dir: &std::path::Path, + cancel: Option<&tokio_util::sync::CancellationToken>, ) -> Option { // Use minute-level granularity to prevent conversation changes every second let timestamp = chrono::Local::now().format("%Y-%m-%d %H:%M:00").to_string(); @@ -3168,7 +3172,9 @@ impl ExtensionManager { // BR-1: give the model a bounded, gitignore-aware map of the workspace so // it doesn't rediscover project structure from scratch every session. The // map is cached and token-capped inside `workspace_summary`. - if let Some(map) = crate::agents::workspace_summary::workspace_summary(working_dir) { + if let Some(map) = + crate::agents::workspace_summary::workspace_summary(working_dir, cancel).await + { content.push('\n'); content.push_str(&map); content.push('\n'); @@ -4280,7 +4286,7 @@ mod tests { let em = ExtensionManager::new_without_provider(temp_dir.path().to_path_buf()); let working_dir = std::path::Path::new("/tmp"); - if let Some(moim) = em.collect_moim("test-session-id", working_dir).await { + if let Some(moim) = em.collect_moim("test-session-id", working_dir, None).await { // Timestamp should end with :00 (seconds fixed to 00) assert!( moim.contains(":00\n"), diff --git a/crates/biorouter/src/agents/moim.rs b/crates/biorouter/src/agents/moim.rs index 9655c0a16..c2d60f62d 100644 --- a/crates/biorouter/src/agents/moim.rs +++ b/crates/biorouter/src/agents/moim.rs @@ -4,6 +4,7 @@ use crate::conversation::message::Message; use crate::conversation::{Conversation, SharedNormalizer}; use rmcp::model::Role; use std::path::Path; +use tokio_util::sync::CancellationToken; // Test-only utility. Do not use in production code. No `test` directive due to call outside crate. thread_local! { @@ -71,13 +72,14 @@ pub async fn inject_moim( extension_manager: &ExtensionManager, working_dir: &Path, normalizer: &SharedNormalizer, + cancel: Option<&CancellationToken>, ) -> (Conversation, bool) { if SKIP.with(|f| f.get()) { return (conversation, false); } if let Some(moim) = extension_manager - .collect_moim(session_id, working_dir) + .collect_moim(session_id, working_dir, cancel) .await { let moim = cap_moim_block(moim, max_moim_tokens()); @@ -142,6 +144,7 @@ mod tests { &em, &working_dir, &SharedNormalizer::new(), + None, ) .await; let msgs = result.messages(); @@ -180,6 +183,7 @@ mod tests { &em, &working_dir, &SharedNormalizer::new(), + None, ) .await; @@ -252,6 +256,7 @@ mod tests { &em, &working_dir, &SharedNormalizer::new(), + None, ) .await; let msgs = result.messages(); @@ -379,6 +384,7 @@ mod tests { &em, &working_dir, &SharedNormalizer::new(), + None, ) .await; diff --git a/crates/biorouter/src/agents/workspace_summary.rs b/crates/biorouter/src/agents/workspace_summary.rs index 8a2def99b..2b80640e9 100644 --- a/crates/biorouter/src/agents/workspace_summary.rs +++ b/crates/biorouter/src/agents/workspace_summary.rs @@ -20,22 +20,77 @@ //! / `_MAX_ENTRIES`). //! - The rendered text is token-capped and coordinates with the BR-2 injection //! budget by reusing [`truncate_to_tokens`]. -//! - Results are cached per working directory and only recomputed when the -//! directory's own mtime changes or a short TTL lapses +//! - Results are cached per working directory for a short TTL //! (`CONTEXT_WORKSPACE_SUMMARY_TTL_SECS`), so a long multi-tool turn walks the //! tree at most once per TTL rather than on every provider call. //! //! The whole feature is gated by `CONTEXT_WORKSPACE_SUMMARY` (default on); set it //! to `false` to fall back to the old one-line behavior. +//! +//! # The walk is never on the turn's critical path +//! +//! This module used to walk the tree **synchronously, on the tokio worker +//! driving the turn**, with no timeout, no cancellation point and no log line. +//! On 2026-09-10 a test drive of `main` measured what that costs: with the +//! default working directory (`$HOME` on macOS) every turn blocked in +//! `std::fs::read_dir` → `__opendir2` under `~/Library/Group Containers` — +//! Dropbox, iCloud, GlobalProtect and Office app-group containers — and never +//! reached the provider at all. 100% of `/usr/bin/sample` samples sat in that +//! one stack, four captures over two sessions; the daemon idled at 0.0% CPU +//! while the composer said "Thinking" for 8m32s. `Stop` could not end it, +//! because a blocking `std::fs` call has no cancellation point, and each wedged +//! turn leaked its worker for the life of the process. +//! +//! Four rules keep that from recurring, and each is load-bearing: +//! +//! 1. **No filesystem syscall happens on the async path.** Not the walk, not the +//! root's `stat`, not `canonicalize`. Every one of them belongs to the +//! blocking task, where a wedge costs a pool thread instead of the turn. This +//! is why the cache is keyed on the path *as given* rather than on its +//! canonical form, and why the fast path is TTL-only. +//! 2. **The walk runs under [`tokio::task::spawn_blocking`] with a finite +//! budget** (`CONTEXT_WORKSPACE_SUMMARY_BUDGET_MS`). When the budget lapses +//! the turn proceeds *without* a map, one WARN names the directory and the +//! lever, and the negative result is cached for the TTL so the next turn pays +//! nothing. The budget cannot be configured to zero: a "disabled" budget +//! would be a way to reinstate the hang. +//! 3. **The wait is tied to the turn's cancellation token**, so `Stop` is +//! honoured immediately even though the walk itself cannot be cancelled. +//! 4. **One walk per root, ever, while one is in flight** (single-flight). A +//! blocked `std::fs` call cannot be cancelled, so a thread that wedges is +//! lost — the single-flight marker is what bounds the loss to **one thread +//! per root** instead of one per turn. A walk that never returns therefore +//! never releases its marker, and that is deliberate. +//! +//! # A home directory is not a workspace +//! +//! Rule 2 bounds the damage; it does not make walking `$HOME` a good idea. A +//! home directory is the union of every project the user owns *and* the +//! operating system's own per-user state — on macOS that state includes the +//! cloud-provider containers that produced the wedge above, and on any platform +//! it is tens of thousands of entries that say nothing about the task at hand. +//! So [`skip_reason`] refuses three kinds of root outright, with no walk at all: +//! the home directory itself, a filesystem root, and anything at or beneath an +//! *opaque tree* (`~/Library`, `~/AppData`, `~/.Trash`, and any path holding a +//! `Group Containers`, `CloudStorage`, `Mobile Documents` or `FileProvider` +//! component). +//! +//! The same trees are pruned *during* a walk that starts somewhere legitimate, +//! and `same_file_system(true)` stops the walk crossing a mount point — a +//! network share or a File Provider volume is a mount, and an unresponsive one +//! is the other way this blocks. +//! +//! See [`docs/agent-loop/workspace-map.md`](../../../../docs/agent-loop/workspace-map.md). use crate::config::paths::Paths; use crate::config::Config; use crate::context_budget::truncate_to_tokens; use once_cell::sync::Lazy; -use std::collections::HashMap; -use std::path::{Path, PathBuf}; +use std::collections::{HashMap, HashSet}; +use std::path::{Component, Path, PathBuf}; use std::sync::Mutex; -use std::time::{Duration, Instant, SystemTime}; +use std::time::{Duration, Instant}; +use tokio_util::sync::CancellationToken; /// Whether the workspace map is injected at all. pub const DEFAULT_ENABLED: bool = true; @@ -45,15 +100,39 @@ pub const DEFAULT_MAX_ENTRIES: usize = 200; pub const DEFAULT_MAX_DEPTH: usize = 3; /// Token cap for the rendered map (coordinates with the BR-2 MOIM budget). pub const DEFAULT_MAX_TOKENS: usize = 2_000; -/// Cache time-to-live: an upper bound on how stale the served map can be when -/// the working directory's own mtime hasn't changed (nested edits). +/// Cache time-to-live: an upper bound on how stale the served map can be. pub const DEFAULT_TTL_SECS: u64 = 30; +/// How long a turn will wait for the walk before giving up on it and proceeding +/// without a map. Generous enough that an ordinary project never notices, short +/// enough that a user does. +pub const DEFAULT_BUDGET_MS: u64 = 1_500; /// Hard ceiling on filesystem entries visited during a single walk, independent /// of `max_entries`, so a pathological directory (100k flat files) can't make /// the bounded walk unbounded. If hit, the map is marked truncated. const SCAN_CAP: usize = 20_000; +/// Path components whose subtree is never walked, wherever they appear. Each is +/// a place the operating system or a sync client mediates reads, so an +/// `opendir` there can block on a daemon rather than on a disk. +const OPAQUE_COMPONENTS: &[&str] = &[ + // macOS app-group containers: Dropbox, iCloud, Office, GlobalProtect. The + // directory that produced the measured wedge. + "Group Containers", + // ~/Library/CloudStorage — every File Provider cloud mount. + "CloudStorage", + // ~/Library/Mobile Documents — iCloud Drive. + "Mobile Documents", + // File Provider caches and domain state. + "FileProvider", + ".Trash", +]; + +/// Children of the home directory that are opaque. Matched home-relative rather +/// than by name alone, because `Library/` is an ordinary directory name inside a +/// project (an R library, a component library) and must stay walkable there. +const HOME_OPAQUE_CHILDREN: &[&str] = &["Library", "AppData", ".Trash"]; + fn config_bool(key: &str, default: bool) -> bool { Config::global().get_param::(key).unwrap_or(default) } @@ -70,6 +149,90 @@ fn enabled() -> bool { config_bool("CONTEXT_WORKSPACE_SUMMARY", DEFAULT_ENABLED) } +fn ttl() -> Duration { + Duration::from_secs(config_u64( + "CONTEXT_WORKSPACE_SUMMARY_TTL_SECS", + DEFAULT_TTL_SECS, + )) +} + +/// The per-turn wait budget. **Always finite**: a configured `0` reads as "use +/// the default", not as "wait forever". `CONTEXT_WORKSPACE_SUMMARY: false` is +/// how the feature is turned off; there is deliberately no setting that puts an +/// uncancellable filesystem walk back on the turn's critical path. +fn budget() -> Duration { + let ms = config_u64("CONTEXT_WORKSPACE_SUMMARY_BUDGET_MS", DEFAULT_BUDGET_MS); + Duration::from_millis(if ms == 0 { DEFAULT_BUDGET_MS } else { ms }) +} + +/// Why a root gets no workspace map at all. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum SkipReason { + /// The working directory *is* the user's home directory. + Home, + /// The working directory is a filesystem root (`/`, `C:\`). + FilesystemRoot, + /// The working directory is at or beneath a tree whose reads are mediated + /// by the OS or a sync client (see [`OPAQUE_COMPONENTS`]). + OpaqueTree, +} + +impl SkipReason { + /// A phrase for the log line, written for whoever is wondering why their + /// chat has no file map. + pub fn describe(self) -> &'static str { + match self { + SkipReason::Home => { + "the working directory is the home directory, which is not a workspace" + } + SkipReason::FilesystemRoot => "the working directory is a filesystem root", + SkipReason::OpaqueTree => { + "the working directory is inside a system or cloud-sync tree whose reads can block" + } + } + } +} + +/// True when `dir` is at or beneath a tree this module refuses to read. +fn is_opaque_tree(dir: &Path, home: Option<&Path>) -> bool { + let opaque_component = dir.components().any(|component| match component { + Component::Normal(name) => OPAQUE_COMPONENTS + .iter() + .any(|opaque| name.eq_ignore_ascii_case(opaque)), + _ => false, + }); + if opaque_component { + return true; + } + let Some(home) = home else { return false }; + HOME_OPAQUE_CHILDREN + .iter() + .any(|child| dir.starts_with(home.join(child))) +} + +/// Whether `dir` gets a workspace map at all, resolved against the real home +/// directory. See the module doc: a home directory is not a workspace. +pub fn skip_reason(dir: &Path) -> Option { + skip_reason_with_home(dir, Paths::home_dir().as_deref()) +} + +/// [`skip_reason`] with the home directory injected, so the rule is unit +/// testable without touching the process environment. +pub fn skip_reason_with_home(dir: &Path, home: Option<&Path>) -> Option { + // `Path`'s equality and `starts_with` both compare *components*, so a + // trailing slash or an interior `.` is already normalised away. + if dir.parent().is_none() { + return Some(SkipReason::FilesystemRoot); + } + if home.is_some_and(|home| dir == home) { + return Some(SkipReason::Home); + } + if is_opaque_tree(dir, home) { + return Some(SkipReason::OpaqueTree); + } + None +} + /// Bounds for a single map render, resolved from config with generous defaults. #[derive(Clone, Copy, Debug)] pub struct SummaryConfig { @@ -100,76 +263,247 @@ impl Default for SummaryConfig { struct CacheEntry { computed_at: Instant, - /// The working directory's own mtime when this entry was computed, if - /// readable. Used as a cheap early-invalidation signal for top-level - /// structural changes (new/removed/renamed direct children). - dir_mtime: Option, /// `None` means "walked, nothing to show" — still cached so an empty or - /// unreadable directory isn't rewalked every action. + /// unreadable directory isn't rewalked every action — or "the walk did not + /// finish inside its budget", which is cached for exactly the same reason. summary: Option, } -static CACHE: Lazy>> = Lazy::new(|| Mutex::new(HashMap::new())); - -/// Clear the cache. Test-only; production relies on TTL/mtime invalidation. -#[cfg(test)] -pub fn clear_cache() { - CACHE.lock().unwrap().clear(); +/// Cache, single-flight markers and warn-once markers under **one** mutex, so +/// there is no lock ordering to get wrong between them. Every critical section +/// here is a few map operations long: the mutex is never held across the walk. +#[derive(Default)] +struct WalkState { + cache: HashMap, + /// Roots with a walk in flight. An entry that never clears is a walk that + /// never returned — see rule 4 in the module doc. + inflight: HashSet, + /// How many walks this process has started per root. Instrumentation: it is + /// what lets a test prove single-flight held, per root rather than + /// process-wide so tests of different roots can run in parallel. + walk_starts: HashMap, + /// Roots that have already produced a budget WARN, so a slow directory + /// costs one log line rather than one per turn. + warned_budget: HashSet, + /// Roots that have already produced a skip log line, same reason. + warned_skip: HashSet, } -fn dir_mtime(dir: &Path) -> Option { - std::fs::metadata(dir).and_then(|m| m.modified()).ok() +static STATE: Lazy> = Lazy::new(|| Mutex::new(WalkState::default())); + +/// Releases the single-flight marker on any *return* from the walk task, +/// including a panic. A thread blocked forever in `opendir` never drops it, +/// which is what bounds the leak to one thread per root. +struct InflightGuard(PathBuf); + +impl Drop for InflightGuard { + fn drop(&mut self) { + if let Ok(mut state) = STATE.lock() { + state.inflight.remove(&self.0); + } + } } /// Absolute path to the global `.biorouterignore`, mirroring the developer MCP /// server so the workspace map honors the same global excludes. +/// +/// Touches the filesystem (`is_file`), so it is resolved inside the blocking +/// task rather than on the turn's async path. fn global_biorouterignore() -> Option { let path = Paths::config_dir().join(".biorouterignore"); path.is_file().then_some(path) } +fn log_skip_once(dir: &Path, reason: SkipReason) { + let first = { + let mut state = STATE.lock().unwrap(); + state.warned_skip.insert(dir.to_path_buf()) + }; + if !first { + return; + } + tracing::info!( + working_dir = %dir.display(), + "workspace map: no file map for this chat because {}. Start the chat in a project \ + directory to get one.", + reason.describe() + ); +} + +fn warn_budget_once(dir: &Path, budget: Duration) { + let first = { + let mut state = STATE.lock().unwrap(); + state.warned_budget.insert(dir.to_path_buf()) + }; + if !first { + return; + } + tracing::warn!( + working_dir = %dir.display(), + budget_ms = budget.as_millis() as u64, + "workspace map: reading this directory did not finish within the budget, so turns here \ + run without a file map. Something under it is slow or unresponsive to read — a network \ + mount or a cloud-sync folder is the usual cause. Set CONTEXT_WORKSPACE_SUMMARY: false in \ + config.yaml to switch the map off, or raise CONTEXT_WORKSPACE_SUMMARY_BUDGET_MS to wait \ + longer." + ); +} + /// The public entry point: a cached, config-bounded workspace map for -/// `working_dir`, or `None` when disabled, empty, or unreadable. -pub fn workspace_summary(working_dir: &Path) -> Option { +/// `working_dir`, or `None` when disabled, skipped, empty, unreadable, or not +/// produced inside this turn's budget. +/// +/// `cancel` is the **turn's** cancellation token. The walk it starts cannot +/// itself be cancelled — no blocking `std::fs` call can — but the *wait* can, +/// so a `Stop` returns from here immediately and leaves the walk to finish (or +/// not) on the blocking pool. +pub async fn workspace_summary( + working_dir: &Path, + cancel: Option<&CancellationToken>, +) -> Option { if !enabled() { return None; } - // Started after the disabled bail so the span covers only real work (this - // does synchronous directory walking on the turn path when the cache misses). + // Started after the disabled bail so the span covers only real work. let _phase = crate::agents::phase_timing::Phase::start("agent.workspace_summary"); - let ttl = Duration::from_secs(config_u64( - "CONTEXT_WORKSPACE_SUMMARY_TTL_SECS", - DEFAULT_TTL_SECS, - )); - let key = std::fs::canonicalize(working_dir).unwrap_or_else(|_| working_dir.to_path_buf()); - let current_mtime = dir_mtime(working_dir); - - // Fast path: serve a cached map when the directory's mtime is unchanged and - // the entry is younger than the TTL. `ttl == 0` disables caching entirely. - if !ttl.is_zero() { - if let Some(entry) = CACHE.lock().unwrap().get(&key) { - let fresh = entry.computed_at.elapsed() < ttl && entry.dir_mtime == current_mtime; - if fresh { + if let Some(reason) = skip_reason(working_dir) { + log_skip_once(working_dir, reason); + return None; + } + + // Both of these read config, which is cheap and does not touch the + // workspace; the walk itself — and every filesystem call it needs, down to + // resolving the global ignore file — is deferred into the closure so it can + // only ever run on the blocking pool. + let cfg = SummaryConfig::from_config(); + let walk_dir = working_dir.to_path_buf(); + summary_bounded( + working_dir.to_path_buf(), + cancel, + ttl(), + budget(), + move || build_summary(&walk_dir, &cfg, global_biorouterignore().as_deref()), + ) + .await +} + +/// The cache, single-flight and budget machinery, with the walk itself injected. +/// +/// Split out from [`workspace_summary`] so a test can supply a walk that +/// provably stalls or provably blocks until released — the two behaviours this +/// exists to survive, and neither of which can be conjured out of a real +/// directory tree on demand. +async fn summary_bounded( + key: PathBuf, + cancel: Option<&CancellationToken>, + ttl: Duration, + budget: Duration, + walk: F, +) -> Option +where + F: FnOnce() -> Option + Send + 'static, +{ + // Fast path, single-flight admission and the stale value in one short + // critical section. Deliberately syscall-free — see rule 1 in the module + // doc. + let stale = { + let mut state = STATE.lock().unwrap(); + if let Some(entry) = state.cache.get(&key) { + if !ttl.is_zero() && entry.computed_at.elapsed() < ttl { return entry.summary.clone(); } } - } + let cached = state + .cache + .get(&key) + .and_then(|entry| entry.summary.clone()); + if state.inflight.contains(&key) { + // A walk of this root is already running. Serving whatever it last + // produced is the whole of the second turn's cost: starting a + // second walk of a root the first one may be wedged on is how a + // single slow directory became one leaked thread per turn. + return cached; + } + state.inflight.insert(key.clone()); + *state.walk_starts.entry(key.clone()).or_insert(0) += 1; + cached + }; - let cfg = SummaryConfig::from_config(); - let summary = build_summary(working_dir, &cfg, global_biorouterignore().as_deref()); - - if !ttl.is_zero() { - CACHE.lock().unwrap().insert( - key, - CacheEntry { - computed_at: Instant::now(), - dir_mtime: current_mtime, - summary: summary.clone(), - }, - ); + let task_key = key.clone(); + let mut handle = tokio::task::spawn_blocking(move || { + let _inflight = InflightGuard(task_key.clone()); + let summary = walk(); + { + let mut state = STATE.lock().unwrap(); + state.cache.insert( + task_key, + CacheEntry { + computed_at: Instant::now(), + summary: summary.clone(), + }, + ); + } + summary + }); + + // `biased` puts cancellation first: a `Stop` must return from here on the + // very next poll, not after whichever branch happens to be ready. + let outcome = match cancel { + Some(token) => tokio::select! { + biased; + _ = token.cancelled() => Wait::Cancelled, + joined = &mut handle => Wait::Finished(joined.ok().flatten()), + _ = tokio::time::sleep(budget) => Wait::OverBudget, + }, + None => tokio::select! { + joined = &mut handle => Wait::Finished(joined.ok().flatten()), + _ = tokio::time::sleep(budget) => Wait::OverBudget, + }, + }; + + match outcome { + Wait::Finished(summary) => summary, + // The turn is going away. Leave the walk running: it owns the + // single-flight marker and will cache its result for the next turn. + Wait::Cancelled => None, + Wait::OverBudget => { + warn_budget_once(&key, budget); + let mut state = STATE.lock().unwrap(); + let landed = state + .cache + .get(&key) + .and_then(|entry| entry.summary.clone()); + if !state.inflight.contains(&key) { + // The walk finished in the gap between the timeout firing and + // this lock. Its result stands; never stamp a negative over it. + return landed; + } + // Cache the negative (or the stale value, if there is one) for the + // TTL so the next turn pays nothing at all. The walk, if it ever + // finishes, overwrites this. + let carry = landed.or(stale); + state.cache.insert( + key, + CacheEntry { + computed_at: Instant::now(), + summary: carry.clone(), + }, + ); + carry + } } - summary +} + +/// How the wait for a walk ended. +enum Wait { + /// The walk returned inside the budget. `None` covers both "nothing to + /// show" and a panicked/aborted task. + Finished(Option), + /// The turn was cancelled. + Cancelled, + /// The budget lapsed with the walk still running. + OverBudget, } /// Filesystem names can contain almost anything on Unix; scrub the two @@ -187,11 +521,15 @@ fn sanitize_name(name: &str) -> String { /// bounded, indented file tree. Pure over its inputs so it is unit-testable /// without the global cache or config. Returns `None` for an empty/unreadable /// directory (nothing worth injecting). +/// +/// **Blocking.** Call it from a blocking context only; [`workspace_summary`] is +/// the async entry point and puts it on the blocking pool. pub fn build_summary( working_dir: &Path, cfg: &SummaryConfig, global_ignore: Option<&Path>, ) -> Option { + let home = Paths::home_dir(); let mut builder = ignore::WalkBuilder::new(working_dir); builder .max_depth(Some(cfg.max_depth.max(1))) @@ -203,6 +541,16 @@ pub fn build_summary( .git_exclude(true) .hidden(true) .parents(true) + // A network share or a File Provider volume is a mount, and reading an + // unresponsive one is the other way this walk blocks. Nothing below a + // project root that lives on another filesystem is worth that risk. + .same_file_system(true) + // The same opaque trees `skip_reason` refuses as roots are pruned here, + // for a walk that started somewhere legitimate and is about to descend + // into one. + .filter_entry(move |entry| { + entry.depth() == 0 || !is_opaque_tree(entry.path(), home.as_deref()) + }) // The agent's own project-ignore file, per-directory during the walk. .add_custom_ignore_filename(".biorouterignore"); if let Some(global) = global_ignore { @@ -278,15 +626,66 @@ pub fn build_summary( Some(truncate_to_tokens(&body, cfg.max_tokens, "workspace map")) } +// --------------------------------------------------------------------------- +// Test-only accessors into the module's process-global state. +// +// Kept together at the bottom, after every production item, because the repo's +// source guards slice a file at its FIRST `#[cfg(test)]` and treat everything +// above it as the production half. An accessor placed mid-file silently +// truncates that view. +// --------------------------------------------------------------------------- + +/// Clear the cache and every marker. Test-only; production relies on TTL +/// invalidation. +#[cfg(test)] +pub fn clear_cache() { + let mut state = STATE.lock().unwrap(); + state.cache.clear(); + state.inflight.clear(); + state.walk_starts.clear(); + state.warned_budget.clear(); + state.warned_skip.clear(); +} + +/// How many walks this process has started for `dir`. Per root, so tests using +/// different temp directories do not observe each other. +#[cfg(test)] +fn walk_starts(dir: &Path) -> usize { + STATE + .lock() + .unwrap() + .walk_starts + .get(dir) + .copied() + .unwrap_or(0) +} + +/// Pretend a walk of `dir` is already running, without starting one. +#[cfg(test)] +fn mark_inflight(dir: &Path) { + STATE.lock().unwrap().inflight.insert(dir.to_path_buf()); +} + #[cfg(test)] mod tests { use super::*; use std::fs; + use std::sync::mpsc; + use std::time::Instant as StdInstant; fn cfg() -> SummaryConfig { SummaryConfig::default() } + /// A root no other test shares, so the process-global cache and + /// single-flight markers cannot make these tests observe each other. + fn unique_key(tag: &str) -> PathBuf { + PathBuf::from(format!( + "/nonexistent/workspace-summary-test/{tag}-{}", + uuid::Uuid::new_v4() + )) + } + #[test] fn test_lists_files_and_dirs() { let dir = tempfile::tempdir().unwrap(); @@ -440,24 +839,294 @@ mod tests { ); } + // ----------------------------------------------------------------------- + // M1: the walk is never on the turn's critical path. + // ----------------------------------------------------------------------- + + /// The finding, in one test: a root whose read never returns must not hold + /// the turn. Before the fix this call was `build_summary` inline on the + /// turn's worker and this test would hang forever rather than fail. + #[tokio::test] + async fn a_stalled_walk_returns_within_the_budget_instead_of_holding_the_turn() { + let key = unique_key("stall"); + // Held for the life of the test so the walk closure blocks exactly as a + // wedged `opendir` does: forever, with no cancellation point. + let (release_tx, release_rx) = mpsc::channel::<()>(); + + let started = StdInstant::now(); + let summary = summary_bounded( + key.clone(), + None, + Duration::from_secs(30), + Duration::from_millis(50), + move || { + // Blocks until the sender is dropped at the end of the test. + let _ = release_rx.recv(); + Some("a map nobody waited for".to_string()) + }, + ) + .await; + let waited = started.elapsed(); + + assert_eq!(summary, None, "the turn proceeds without a map"); + assert!( + waited < Duration::from_secs(5), + "returned in {waited:?}; the budget was 50ms, so this waited for the walk" + ); + drop(release_tx); + } + + /// The negative is cached, so a second turn inside the TTL pays nothing at + /// all — not even the budget. + #[tokio::test] + async fn a_lapsed_budget_is_cached_so_the_next_turn_does_not_pay_again() { + let key = unique_key("negative-cache"); + let (release_tx, release_rx) = mpsc::channel::<()>(); + + let first = summary_bounded( + key.clone(), + None, + Duration::from_secs(30), + Duration::from_millis(50), + move || { + let _ = release_rx.recv(); + None + }, + ) + .await; + assert_eq!(first, None); + + let started = StdInstant::now(); + let second = summary_bounded( + key.clone(), + None, + Duration::from_secs(30), + Duration::from_millis(50), + || panic!("a second walk must not be started"), + ) + .await; + let waited = started.elapsed(); + + assert_eq!(second, None); + assert!( + waited < Duration::from_millis(50), + "served from the negative cache, so it never reached the budget: {waited:?}" + ); + assert_eq!(walk_starts(&key), 1, "exactly one walk was ever started"); + drop(release_tx); + } + + /// A blocked `std::fs` call cannot be cancelled, so the thread it is on is + /// lost. Single-flight is what bounds that loss to one thread per root + /// rather than one per turn. + #[tokio::test] + async fn a_second_turn_never_starts_a_second_walk_of_the_same_root() { + let key = unique_key("single-flight"); + mark_inflight(&key); + + let summary = summary_bounded( + key.clone(), + None, + Duration::from_secs(30), + Duration::from_secs(30), + || panic!("single-flight failed: a second walk of this root was started"), + ) + .await; + + assert_eq!(summary, None, "nothing cached yet, so nothing to serve"); + assert_eq!(walk_starts(&key), 0, "no walk was started"); + } + + /// While one walk is in flight, a second turn is served whatever the last + /// completed walk produced rather than being made to wait or to re-walk. + #[tokio::test] + async fn an_inflight_root_serves_the_last_good_map_to_the_next_turn() { + let key = unique_key("serve-stale"); + + let first = summary_bounded( + key.clone(), + None, + Duration::from_millis(1), + Duration::from_secs(30), + || Some("first map".to_string()), + ) + .await; + assert_eq!(first.as_deref(), Some("first map")); + + // The TTL above has already lapsed, so the next call would ordinarily + // re-walk; the in-flight marker says another one is already running. + mark_inflight(&key); + let second = summary_bounded( + key.clone(), + None, + Duration::from_millis(1), + Duration::from_secs(30), + || panic!("single-flight failed"), + ) + .await; + + assert_eq!( + second.as_deref(), + Some("first map"), + "the last good map is served rather than nothing" + ); + assert_eq!(walk_starts(&key), 1); + } + + /// M2's other half: `Stop` must be honoured immediately. The walk cannot be + /// cancelled, but the wait for it can. + #[tokio::test] + async fn a_cancelled_turn_returns_immediately_and_leaves_the_walk_running() { + let key = unique_key("cancel"); + let cancel = CancellationToken::new(); + cancel.cancel(); + let (release_tx, release_rx) = mpsc::channel::<()>(); + + let started = StdInstant::now(); + let summary = summary_bounded( + key.clone(), + Some(&cancel), + Duration::from_secs(30), + Duration::from_secs(30), + move || { + let _ = release_rx.recv(); + Some("too late".to_string()) + }, + ) + .await; + let waited = started.elapsed(); + + assert_eq!(summary, None); + assert!( + waited < Duration::from_secs(1), + "cancellation returned in {waited:?}, not after the 30s budget" + ); + drop(release_tx); + } + + /// A walk that finishes inside the budget is served, and cached for the TTL. + #[tokio::test] + async fn a_fast_walk_is_served_and_then_cached_for_the_ttl() { + let key = unique_key("happy"); + + let first = summary_bounded( + key.clone(), + None, + Duration::from_secs(30), + Duration::from_secs(30), + || Some("the map".to_string()), + ) + .await; + assert_eq!(first.as_deref(), Some("the map")); + + let second = summary_bounded( + key.clone(), + None, + Duration::from_secs(30), + Duration::from_secs(30), + || panic!("a cached map must not be re-walked inside the TTL"), + ) + .await; + assert_eq!(second.as_deref(), Some("the map")); + assert_eq!(walk_starts(&key), 1); + } + + // ----------------------------------------------------------------------- + // M1: a home directory is not a workspace. + // ----------------------------------------------------------------------- + #[test] - fn test_cache_serves_and_invalidates_on_mtime() { - clear_cache(); - let dir = tempfile::tempdir().unwrap(); - fs::write(dir.path().join("one.txt"), "x").unwrap(); + fn the_home_directory_itself_gets_no_map() { + let home = Path::new("/Users/example"); + assert_eq!( + skip_reason_with_home(home, Some(home)), + Some(SkipReason::Home) + ); + // A trailing slash is the same directory. + assert_eq!( + skip_reason_with_home(Path::new("/Users/example/"), Some(home)), + Some(SkipReason::Home) + ); + // A project inside it is fine. + assert_eq!( + skip_reason_with_home(Path::new("/Users/example/code/thing"), Some(home)), + None + ); + } + + #[test] + fn a_filesystem_root_gets_no_map() { + assert_eq!( + skip_reason_with_home(Path::new("/"), Some(Path::new("/Users/example"))), + Some(SkipReason::FilesystemRoot) + ); + } - let first = workspace_summary(dir.path()).expect("first summary"); - assert!(first.contains("one.txt")); + #[test] + fn the_directories_that_produced_the_wedge_get_no_map() { + let home = Path::new("/Users/example"); + for path in [ + "/Users/example/Library", + "/Users/example/Library/Group Containers", + "/Users/example/Library/Group Containers/group.com.apple.CloudDocs", + "/Users/example/Library/CloudStorage/Dropbox", + "/Users/example/Library/Mobile Documents/com~apple~CloudDocs", + "/Users/example/AppData/Local", + "/Users/example/.Trash", + ] { + assert_eq!( + skip_reason_with_home(Path::new(path), Some(home)), + Some(SkipReason::OpaqueTree), + "{path} must get no workspace map" + ); + } + } + + #[test] + fn a_project_directory_named_library_is_still_walkable() { + let home = Path::new("/Users/example"); + // The `Library` rule is home-relative on purpose: an R project, or a + // component library, may hold a directory of that name. + assert_eq!( + skip_reason_with_home(Path::new("/Users/example/code/app/Library"), Some(home)), + None + ); + assert_eq!( + skip_reason_with_home(Path::new("/srv/project/library"), Some(home)), + None + ); + } - // Add a top-level file; the directory mtime changes, so the cache must - // invalidate and pick up the new entry even within the TTL. - fs::write(dir.path().join("two.txt"), "y").unwrap(); - // mtime resolution can be coarse; nudge the directory to be safe. - let refreshed = workspace_summary(dir.path()).expect("second summary"); + #[test] + fn an_opaque_subtree_is_pruned_from_a_walk_that_started_somewhere_legitimate() { + let dir = tempfile::tempdir().unwrap(); + fs::write(dir.path().join("keep.txt"), "x").unwrap(); + fs::create_dir_all(dir.path().join("Group Containers/group.com.example")).unwrap(); + fs::write( + dir.path() + .join("Group Containers/group.com.example/blocked.txt"), + "x", + ) + .unwrap(); + + let out = build_summary(dir.path(), &cfg(), None).expect("summary"); + assert!(out.contains("keep.txt")); assert!( - refreshed.contains("two.txt") || first.contains("two.txt"), - "new top-level file surfaces after mtime change" + !out.contains("blocked.txt"), + "nothing under an opaque component is read: {out}" ); - clear_cache(); + assert!( + !out.contains("group.com.example"), + "the opaque tree is not descended into: {out}" + ); + } + + #[test] + fn the_budget_can_never_be_configured_to_zero() { + // A zero budget would be a way to put an uncancellable filesystem walk + // back on the turn's critical path, which is the whole bug. The only + // supported "off" is CONTEXT_WORKSPACE_SUMMARY: false. + assert!(!budget().is_zero()); + assert_eq!(DEFAULT_BUDGET_MS, 1_500); } } diff --git a/docs/agent-loop/README.md b/docs/agent-loop/README.md index 94cbd2fc6..4f43178c6 100644 --- a/docs/agent-loop/README.md +++ b/docs/agent-loop/README.md @@ -36,6 +36,7 @@ table repeats it, so check that before treating a design as a description of shi | [The compaction preservation marker](compaction-preservation-marker.md) | The per-message marker that carries a message verbatim through every compaction instead of dissolving it into a summary: the exact promise ("never dropped by summarization", not "never dropped"), the single funnel all five compaction sites reach, the two caps that bound the pinned set, oldest-first eviction, and the three signals that make exceeding the bound observable. Current; no producer sets the marker yet. | | [Tool routing](tool-routing.md) | Which tool the agent should reach for and in what order of preference: the two-tier model, a per-tool "when to use and when not to", the overlap matrix, the three places in source where the same guidance is mirrored, and the always-on `tool_result` log line that makes every dispatched call inspectable. Current, with the tier reading awaiting confirmation and a deprecation proposal awaiting approval. | | [Turn cancellation and process reaping](turn-cancellation-and-process-reaping.md) | How Stop reaches a running OS process: the cooperative chain from `/agent/cancel` to `killpg`, the extra hop a `code_execution`-nested tool call adds, and the two rules that keep it intact — never abort a future that owes a cancellation downstream, and never assume killing a child kills its descendants. Current; written up after [#72](https://github.com/BaranziniLab/biorouter/issues/72). | +| [The workspace map](workspace-map.md) | The per-turn file map of the working directory: what it contains and what bounds it, the four rules that keep its directory walk off the turn's critical path (no syscall on the async path, a finite budget on the blocking pool, a wait tied to the turn's cancellation token, one walk per root), and the decision that a home directory, a filesystem root and the OS/cloud-sync trees get no map at all. Current; the bounding rules landed after finding M1 of the 2026-09-10 test drive measured every turn blocking in `opendir` under `~/Library/Group Containers`. | ## Subfolders diff --git a/docs/agent-loop/workspace-map.md b/docs/agent-loop/workspace-map.md new file mode 100644 index 000000000..a2d983604 --- /dev/null +++ b/docs/agent-loop/workspace-map.md @@ -0,0 +1,156 @@ +# The workspace map + +> **What this is.** The per-turn file map the agent is given about its working directory: what it +> contains, what it costs, the four rules that keep its directory walk off the turn's critical +> path, and the decision that a home directory gets no map at all. +> **Status:** Current. The bounding rules landed after finding M1 of the 2026-09-10 test drive of +> `main`; the map itself is BR-1 and predates them. +> **Audience:** Contributors changing per-turn context assembly, and anyone diagnosing a chat that +> hangs before the model is reached. + +## What the map is + +Every turn, the agent is handed a short `` block — the "message of the moment", or MOIM — +carrying the current time, the working directory, and a bounded, gitignore-aware listing of that +directory. Without it the agent knows one thing about its surroundings, the working directory's +path, and rediscovers project structure by hand in every session. + +The listing is produced by `agents::workspace_summary` and appended by `ExtensionManager::collect_moim` +right after the working-directory line. It is names only, depth-capped, entry-capped, token-capped and +cached, and it honours the same trust boundary as the rest of the agent's file access: `.gitignore`, +`.ignore`, and the local and global `.biorouterignore` files. Anything the user hid from the agent +stays hidden here too. + +`CONTEXT_WORKSPACE_SUMMARY: false` in `config.yaml` turns the whole feature off and restores the old +one-line behaviour. The setting is read live, through the file-stamp-cached config, so it takes +effect without restarting the daemon. + +## The bounds + +| Setting | Default | What it bounds | +|---|---|---| +| `CONTEXT_WORKSPACE_SUMMARY` | `true` | Whether a map is injected at all. | +| `CONTEXT_WORKSPACE_SUMMARY_MAX_DEPTH` | `3` | How far below the working directory the walk descends. | +| `CONTEXT_WORKSPACE_SUMMARY_MAX_ENTRIES` | `200` | Rendered entries before the tail is elided. | +| `CONTEXT_WORKSPACE_SUMMARY_MAX_TOKENS` | `2000` | The rendered map's own token budget. | +| `CONTEXT_WORKSPACE_SUMMARY_TTL_SECS` | `30` | How stale a served map may be. | +| `CONTEXT_WORKSPACE_SUMMARY_BUDGET_MS` | `1500` | How long a turn waits for the walk before proceeding without a map. | + +`SCAN_CAP` (20,000 entries) is a hard ceiling in source rather than a setting, so a directory holding +100,000 flat files cannot make the bounded walk unbounded. + +## Why the walk is never on the turn's critical path + +This module used to walk the tree **synchronously, on the tokio worker driving the turn**, with no +timeout, no cancellation point and no log line. On 2026-09-10 a test drive of `main` measured what +that costs. With the default working directory — `$HOME` on macOS — every turn blocked in +`std::fs::read_dir` → `__opendir2` under `~/Library/Group Containers`, which holds the Dropbox, +iCloud, GlobalProtect and Office app-group containers, and never reached the provider at all. One +hundred percent of `/usr/bin/sample` samples sat in that one stack across four captures in two +sessions. The daemon idled at 0.0% CPU while the composer said "Thinking" for eight and a half +minutes. `Stop` could not end it, because a blocking `std::fs` call has no cancellation point, and +each wedged turn leaked its worker for the life of the process — the thread and its three open +descriptors were still there twenty minutes later, after the chat had been deleted through the +interface. + +A control ruled out the obvious explanation: a shell `find` over the same directory finished in 3.4 +seconds and returned 11,849 entries. Only that process could not read it. Code signing was +investigated, acted on and falsified — re-signing the daemon with the Developer ID changed nothing. + +Four rules now keep that from recurring. Each is load-bearing, and each is pinned by a test in +`agents::workspace_summary`. + +**No filesystem syscall happens on the async path.** Not the walk, not the root's `stat`, not +`canonicalize`. Every one of them belongs to the blocking task, where a wedge costs a pool thread +instead of the turn. Two consequences follow that look like oversights and are not: the cache is +keyed on the working directory *as given* rather than on its canonical form, so two spellings of one +directory get two entries; and the fast path is TTL-only. + +**The walk runs on the blocking pool with a finite budget.** When the budget lapses the turn proceeds +without a map, one WARN names the directory and the lever, and the negative result is cached for the +TTL so the next turn pays nothing at all. The budget cannot be configured to zero — a zero would read +as "wait forever", which is the bug. `CONTEXT_WORKSPACE_SUMMARY: false` is the only supported off +switch. + +**The wait is tied to the turn's cancellation token.** The walk itself cannot be cancelled; the wait +for it can, so `Stop` returns immediately and leaves the walk to finish, or not, on its own. This is +why the token is threaded from `Agent::reply_internal` down through `assemble_turn_context`, +`inject_moim` and `collect_moim` — the walk is the only part of per-turn context assembly that +touches the filesystem. + +**One walk per root while one is in flight.** A blocked `std::fs` call cannot be cancelled, so a +thread that wedges is lost. Single-flight is what bounds that loss to one thread per root instead of +one per turn. A walk that never returns therefore never releases its marker, and that is deliberate: +the alternative is a new doomed thread on every turn. While a walk is outstanding, later turns are +served whatever the last completed walk produced. + +## A home directory is not a workspace + +The rules above bound the damage. They do not make walking `$HOME` a good idea, and the decision here +is that it is not one. + +A home directory is the union of every project the user owns *and* the operating system's own +per-user state. On macOS that state includes the cloud-provider containers that produced the wedge. +On any platform it is tens of thousands of entries that say nothing about the task at hand, so even +a walk that completes spends the map's entire 200-entry budget on `Desktop/`, `Downloads/` and +`Library/` before reaching anything the agent could use. + +So `skip_reason` refuses three kinds of root outright, with no walk at all and no budget spent: + +- **The home directory itself.** Not directories inside it: `~/code/thing` is an ordinary workspace. +- **A filesystem root** (`/`, `C:\`), for the same reason and more so. +- **Anything at or beneath an opaque tree** — `~/Library`, `~/AppData`, `~/.Trash`, and any path + holding a `Group Containers`, `CloudStorage`, `Mobile Documents` or `FileProvider` component. + These are places where the operating system or a sync client mediates the read, so an `opendir` + can block on a daemon rather than on a disk. + +The `Library` and `AppData` rules are matched **home-relative**, not by name alone. `Library/` is an +ordinary directory name inside a project — an R library, a component library — and stays walkable +there. + +The same opaque trees are pruned *during* a walk that started somewhere legitimate, and the walk does +not cross a mount point: a network share and a File Provider volume are both mounts, and an +unresponsive one is the other way this blocks. + +A skipped root logs one line naming the reason. The chat still works; it simply has no file map, and +the agent's directory tools reach the tree on demand exactly as they did before BR-1. + +### Why not just default the feature off for `$HOME`? + +That was the alternative considered, and it is weaker in both directions. Turning +`CONTEXT_WORKSPACE_SUMMARY` off for one root would leave the setting reading `true` while behaving as +`false`, which is the kind of divergence that costs an afternoon to diagnose; and it would say +nothing about `~/Library/CloudStorage`, which is not the home directory and wedges just as hard. +Refusing the root is narrower, states the rule where the rule belongs, and leaves the feature switch +meaning exactly one thing. + +## What a turn without a map looks like + +Nothing in the transcript. The MOIM block simply carries the working-directory line and no listing, +which is the pre-BR-1 behaviour. The evidence is in the daemon log, once per directory per process: + +- `workspace map: no file map for this chat because …` at INFO, for a refused root. +- `workspace map: reading this directory did not finish within the budget …` at WARN, naming the + directory and both levers, for a walk that overran. + +## Tests + +```bash +cargo test -p biorouter --lib -- workspace_summary +``` + +The behavioural tests inject the walk rather than trying to conjure a stalling directory on demand, +because a directory that reliably blocks `opendir` cannot be created from a test: a FIFO does not +stall a walk (`walkdir` only `lstat`s it), and an unresponsive mount is not something a unit test may +arrange. `summary_bounded` therefore takes the walk as a closure, and the tests supply one that +blocks until released. That makes the budget, cancellation and single-flight assertions deterministic +instead of racing a real tree. + +## Related documentation + +- [Turn cancellation and process reaping](turn-cancellation-and-process-reaping.md) — the rest of + what `Stop` reaches, and the two rules that keep the cancellation chain intact. +- [Context engineering](context-engineering.md) — the other sources of durable background knowledge + the agent is given. +- [Workspace control](workspace-control.md) — the agent's tool surface over *other* conversations, + which is a different thing that shares the word. From 5c6a5f80d75889798394365db2534ca57257deb3 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Thu, 10 Sep 2026 11:17:52 -0700 Subject: [PATCH 2/3] fix(agent): keep TTL=0 meaning "no caching", and drop a dead test helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things a self-review caught in the previous commit. The walk task cached its result unconditionally, so a configured CONTEXT_WORKSPACE_SUMMARY_TTL_SECS of 0 — which has always meant "cache nothing" — would still leave an entry a later turn could be served off the over-budget path. Both the task's write and the over-budget stamp now respect it, so the setting means one thing again. clear_cache had no caller left: the mtime test that used it is gone, and the new tests deliberately key off a root of their own instead, because clearing process-global state from one test breaks whichever test is running beside it. Said so where the helper used to be. --- .../biorouter/src/agents/workspace_summary.rs | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/crates/biorouter/src/agents/workspace_summary.rs b/crates/biorouter/src/agents/workspace_summary.rs index 2b80640e9..5a9be4332 100644 --- a/crates/biorouter/src/agents/workspace_summary.rs +++ b/crates/biorouter/src/agents/workspace_summary.rs @@ -434,7 +434,9 @@ where let mut handle = tokio::task::spawn_blocking(move || { let _inflight = InflightGuard(task_key.clone()); let summary = walk(); - { + // `ttl == 0` disables caching entirely, as it always has: with no entry + // written there is nothing for a later turn to serve. + if !ttl.is_zero() { let mut state = STATE.lock().unwrap(); state.cache.insert( task_key, @@ -469,6 +471,11 @@ where Wait::Cancelled => None, Wait::OverBudget => { warn_budget_once(&key, budget); + if ttl.is_zero() { + // Caching is off, so there is nothing to stamp and nothing to + // serve. The walk keeps its single-flight marker regardless. + return None; + } let mut state = STATE.lock().unwrap(); let landed = state .cache @@ -627,7 +634,10 @@ pub fn build_summary( } // --------------------------------------------------------------------------- -// Test-only accessors into the module's process-global state. +// Test-only accessors into the module's process-global state. There is +// deliberately no `clear_cache`: every test keys off a root of its own +// (`unique_key`), because clearing shared state from one test breaks whichever +// other test is running beside it. // // Kept together at the bottom, after every production item, because the repo's // source guards slice a file at its FIRST `#[cfg(test)]` and treat everything @@ -635,18 +645,6 @@ pub fn build_summary( // truncates that view. // --------------------------------------------------------------------------- -/// Clear the cache and every marker. Test-only; production relies on TTL -/// invalidation. -#[cfg(test)] -pub fn clear_cache() { - let mut state = STATE.lock().unwrap(); - state.cache.clear(); - state.inflight.clear(); - state.walk_starts.clear(); - state.warned_budget.clear(); - state.warned_skip.clear(); -} - /// How many walks this process has started for `dir`. Per root, so tests using /// different temp directories do not observe each other. #[cfg(test)] From 6817ef02039e26b61a7b56a0c4a8df563bee0010 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Thu, 10 Sep 2026 11:33:36 -0700 Subject: [PATCH 3/3] =?UTF-8?q?fix(agent):=20do=20not=20treat=20~/AppData?= =?UTF-8?q?=20as=20opaque=20=E2=80=94=20it=20holds=20Windows=20temp=20dirs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AppData went onto the opaque-roots list as the obvious Windows counterpart to ~/Library. It is not one. std::env::temp_dir() on Windows is %USERPROFILE%\AppData\Local\Temp, so refusing that subtree refuses every scratch workspace on one platform and not the other two — and empties the walk in every build_summary test that runs there, since they all build their fixture with tempfile::tempdir(). Nothing under AppData blocks the way a File Provider mount does. The component rules (Group Containers, CloudStorage, Mobile Documents, FileProvider) are what capture the measured hazard, and they match wherever they appear, Windows included. Caught by reading, not by CI, and the reason is worth recording: the Windows runner's TEMP is C:\Users\RUNNER~1\... against a USERPROFILE of C:\Users\runneradmin, so the 8.3 short name means the prefix test might not have matched and the job could have passed on luck. Adds the regression test both ways: skip_reason on this platform's real temp dir, and a Windows-shaped scratch path under an injected home, so the rule is asserted where the platform cannot hide it. Reverting the list and simulating the Windows layout fails it with `left: Some(OpaqueTree) right: None`. --- .../biorouter/src/agents/workspace_summary.rs | 41 ++++++++++++++++++- docs/agent-loop/workspace-map.md | 15 ++++--- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/crates/biorouter/src/agents/workspace_summary.rs b/crates/biorouter/src/agents/workspace_summary.rs index 5a9be4332..2f8167053 100644 --- a/crates/biorouter/src/agents/workspace_summary.rs +++ b/crates/biorouter/src/agents/workspace_summary.rs @@ -131,7 +131,15 @@ const OPAQUE_COMPONENTS: &[&str] = &[ /// Children of the home directory that are opaque. Matched home-relative rather /// than by name alone, because `Library/` is an ordinary directory name inside a /// project (an R library, a component library) and must stay walkable there. -const HOME_OPAQUE_CHILDREN: &[&str] = &["Library", "AppData", ".Trash"]; +/// +/// ⚠ `AppData` is deliberately NOT on this list, though it is the obvious +/// Windows counterpart to `~/Library`. `std::env::temp_dir()` on Windows is +/// `%USERPROFILE%\AppData\Local\Temp`, so refusing that subtree would refuse +/// every scratch workspace — and empty the walk in every `build_summary` test +/// that runs there. Nothing in `AppData` blocks the way a File Provider mount +/// does; the components above are what capture the measured hazard, and they +/// are matched wherever they appear, including on Windows. +const HOME_OPAQUE_CHILDREN: &[&str] = &["Library", ".Trash"]; fn config_bool(key: &str, default: bool) -> bool { Config::global().get_param::(key).unwrap_or(default) @@ -1069,7 +1077,6 @@ mod tests { "/Users/example/Library/Group Containers/group.com.apple.CloudDocs", "/Users/example/Library/CloudStorage/Dropbox", "/Users/example/Library/Mobile Documents/com~apple~CloudDocs", - "/Users/example/AppData/Local", "/Users/example/.Trash", ] { assert_eq!( @@ -1119,6 +1126,36 @@ mod tests { ); } + /// A scratch directory is an ordinary workspace, and on Windows every one of + /// them lives under `%USERPROFILE%\AppData\Local\Temp`. An `AppData` entry + /// in the opaque list would therefore refuse a legitimate root on one + /// platform and not the other two, and would empty the walk in every + /// `build_summary` test above — which is how this was caught. + #[test] + fn a_temporary_directory_is_a_legitimate_workspace_on_every_platform() { + let dir = tempfile::tempdir().unwrap(); + assert_eq!( + skip_reason(dir.path()), + None, + "a temp dir must get a workspace map: {}", + dir.path().display() + ); + // The same rule, stated without depending on where this platform puts + // its temp directory: a Windows-shaped scratch path under the home + // directory is a workspace like any other. + let home = Path::new("/Users/example"); + assert_eq!( + skip_reason_with_home( + Path::new("/Users/example/AppData/Local/Temp/.tmpABC123"), + Some(home) + ), + None + ); + std::fs::write(dir.path().join("kept.txt"), "x").unwrap(); + let out = build_summary(dir.path(), &cfg(), None).expect("temp dirs are walked"); + assert!(out.contains("kept.txt")); + } + #[test] fn the_budget_can_never_be_configured_to_zero() { // A zero budget would be a way to put an uncancellable filesystem walk diff --git a/docs/agent-loop/workspace-map.md b/docs/agent-loop/workspace-map.md index a2d983604..883df6cc0 100644 --- a/docs/agent-loop/workspace-map.md +++ b/docs/agent-loop/workspace-map.md @@ -99,14 +99,19 @@ So `skip_reason` refuses three kinds of root outright, with no walk at all and n - **The home directory itself.** Not directories inside it: `~/code/thing` is an ordinary workspace. - **A filesystem root** (`/`, `C:\`), for the same reason and more so. -- **Anything at or beneath an opaque tree** — `~/Library`, `~/AppData`, `~/.Trash`, and any path - holding a `Group Containers`, `CloudStorage`, `Mobile Documents` or `FileProvider` component. +- **Anything at or beneath an opaque tree** — `~/Library`, `~/.Trash`, and any path holding a + `Group Containers`, `CloudStorage`, `Mobile Documents` or `FileProvider` component. These are places where the operating system or a sync client mediates the read, so an `opendir` can block on a daemon rather than on a disk. -The `Library` and `AppData` rules are matched **home-relative**, not by name alone. `Library/` is an -ordinary directory name inside a project — an R library, a component library — and stays walkable -there. +The `Library` rule is matched **home-relative**, not by name alone. `Library/` is an ordinary +directory name inside a project — an R library, a component library — and stays walkable there. + +`AppData` is deliberately *not* on the list, though it is the obvious Windows counterpart to +`~/Library`. `std::env::temp_dir()` on Windows is `%USERPROFILE%\AppData\Local\Temp`, so refusing +that subtree would refuse every scratch workspace on one platform and not the other two. Nothing in +`AppData` blocks the way a File Provider mount does, and the components above capture the measured +hazard wherever they appear, Windows included. The same opaque trees are pruned *during* a walk that started somewhere legitimate, and the walk does not cross a mount point: a network share and a File Provider volume are both mounts, and an