From 246daf4664c86678a32d2366f5fcabc71c43c29c Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Wed, 9 Sep 2026 16:52:41 -0700 Subject: [PATCH 1/3] fix(session-meta): prune the row map to the union of every live watcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SessionMetaEvents::last` is process-global; a poll's `ids` list is one window's. `retain_watched(&ids)` pruned the shared map against whichever caller's list arrived last, so two Biorouter windows with different chats open evicted each other every 2 s — and `observe` adopts a first-seen id SILENTLY, so each eviction was followed by a silent re-adoption and neither window was ever told a row had moved. A poll now takes a `WatchGuard` for the life of the request. The guard's Drop retires the claim and prunes to the UNION of every live watcher, which is the only list that agrees with the map it prunes. `retain_watched` is gone; `retain_union` is private and reachable only from the drop. The union alone is not enough: a claim lives for exactly one parked GET, so the only window's ids are unclaimed for an instant at every request boundary, and pruning strictly to the union there would evict the ids that window is about to ask for again. `RETENTION` (5 min, and an id's stamp is refreshed by every read) closes that gap; a test constructs the feed with `Duration::ZERO` to observe an eviction without sleeping. Measured on the pre-fix tree with the regression ported onto the old API: "window A must learn that s1 moved; left: 0, right: 1". --- .../src/routes/session_meta.rs | 27 +- crates/biorouter/src/session_meta.rs | 272 ++++++++++++++++-- 2 files changed, 262 insertions(+), 37 deletions(-) diff --git a/crates/biorouter-server/src/routes/session_meta.rs b/crates/biorouter-server/src/routes/session_meta.rs index caec804c2..27e398327 100644 --- a/crates/biorouter-server/src/routes/session_meta.rs +++ b/crates/biorouter-server/src/routes/session_meta.rs @@ -21,11 +21,18 @@ //! //! # The poll does the reading //! -//! There is no background watcher and no registry of watched ids. The request -//! carries the ids its client has open, and while parked it re-reads exactly -//! those rows on a short interval, hands them to -//! [`SessionMetaEvents::observe`], and returns as soon as the revision moves. -//! An idle app with no chats open therefore reads nothing at all. +//! There is no background watcher. The request carries the ids its client has +//! open, and while parked it re-reads exactly those rows on a short interval, +//! hands them to [`SessionMetaEvents::observe`], and returns as soon as the +//! revision moves. An idle app with no chats open therefore reads nothing at +//! all. +//! +//! ⚠ **A poll does register its ids, and that registry is not bookkeeping.** +//! The row map behind `observe` is process-global while an id list is one +//! window's, so a poll that pruned that map against its OWN list would evict a +//! second window's chats — and a re-adopted id is adopted SILENTLY, so the two +//! windows would thrash and neither would ever be told a row moved. The claim +//! taken below lives for exactly this request; see [`SessionMetaEvents::watch`]. //! //! ⚠ **`since=0` is a baseline request, not a replay.** A client establishing //! itself gets the current revision and no changes; asking for a replay from @@ -126,6 +133,15 @@ pub async fn session_changes( .unwrap_or(MAX_WAIT) .min(MAX_WAIT); + // Claimed for the life of this poll and released when it answers, so the + // row map is pruned against the union of every live watcher rather than + // against whichever list arrived last. + // + // ⚠ It must be a NAMED binding. `let _ = events.watch(&ids)` drops the + // guard on the spot and reinstates the defect in a shape that reads as a + // fix. + let _claim = events.watch(&ids); + // Adopt this caller's ids before parking. A chat opened a moment ago has not // changed, and reporting its whole row as new would wake every window on // connect — the first observation of an id is silent by construction @@ -142,7 +158,6 @@ pub async fn session_changes( { events.observe(rows); } - events.retain_watched(&ids); let delta = events.since(query.since); if !delta.changes.is_empty() || delta.truncated { diff --git a/crates/biorouter/src/session_meta.rs b/crates/biorouter/src/session_meta.rs index 8e8865dd4..ba18ce9c4 100644 --- a/crates/biorouter/src/session_meta.rs +++ b/crates/biorouter/src/session_meta.rs @@ -58,10 +58,11 @@ //! believes a change's fields and never refetches drifts the first time two //! changes race. Every consumer here re-reads the row it was told about. -use std::collections::HashMap; +use std::collections::hash_map::Entry; +use std::collections::{HashMap, HashSet}; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex, PoisonError}; -use std::time::Duration; +use std::time::{Duration, Instant}; use serde::{Deserialize, Serialize}; use tokio::sync::Notify; @@ -75,6 +76,19 @@ use tracing::debug; /// client that has been away long enough to want a fresh read anyway. const BUFFER: usize = 32; +/// How long a chat's row survives in [`SessionMetaEvents::last`] after the last +/// watcher holding its id has gone away. +/// +/// ⚠ **A grace period, and it is what makes pruning SAFE rather than tidy.** A +/// client's claim lives for exactly one parked GET, so between two consecutive +/// polls of the only open window there is an instant with no live watcher at +/// all. Pruning strictly to the live union at that instant would evict the very +/// ids that window is about to ask about again — and [`SessionMetaEvents::observe`] +/// adopts a first-seen id SILENTLY, so a row rewritten in that gap would be +/// swallowed for good. Five minutes is far longer than any request boundary and +/// far shorter than a daemon's life. +const RETENTION: Duration = Duration::from_secs(300); + /// What changed about one chat's row. /// /// ⚠ **Advisory, exactly like the frame in `/reply`.** It is not a session @@ -121,6 +135,15 @@ pub struct SessionMetaRow { pub privacy_reason: Option, } +/// One watched chat's row, and when this process last read it. +/// +/// The timestamp is the second half of the retention rule: an id survives while +/// a live watcher holds it, and for [`RETENTION`] after the last one lets go. +struct Tracked { + row: SessionMetaRow, + seen: Instant, +} + #[derive(Default)] struct Buffer { changes: Vec, @@ -140,17 +163,31 @@ pub struct SessionMetaEvents { /// does at startup: a client that has just opened a chat has not changed it, /// and reporting the whole row as new would make every client refetch on /// connect for nothing. - last: Mutex>, + last: Mutex>, + /// The open-chat ids of every LIVE watcher, keyed by its token. + /// + /// ⚠ **This map is process-global; one caller's id list is not.** Two + /// Biorouter windows (tab tear-off is a shipped feature) each mount one + /// subscription and poll with DIFFERENT open-chat sets, so pruning + /// [`Self::last`] against the ids of whichever poll happened to arrive last + /// makes the two windows evict each other in turn: B's prune drops A's + /// chats, A's next `observe` re-adopts them SILENTLY and publishes nothing, + /// then A's prune drops B's. Neither window ever learns that a row moved. + /// The union of every live watcher is the only list that is safe to prune + /// against, which is why the registry exists at all. + watchers: Mutex>>, + /// Hands out the tokens above. Monotonic and never reused, so a guard can + /// only ever retire its own claim. + next_watcher: AtomicU64, + /// How long an id survives after its last watcher goes away — [`RETENTION`] + /// in production, and settable so a test can observe an eviction without + /// sleeping through it. + retention: Duration, } impl Default for SessionMetaEvents { fn default() -> Self { - Self { - revision: AtomicU64::new(0), - buffer: Mutex::new(Buffer::default()), - notify: Notify::new(), - last: Mutex::new(HashMap::new()), - } + Self::with_retention(RETENTION) } } @@ -161,6 +198,22 @@ impl SessionMetaEvents { &INSTANCE } + /// A feed whose unwatched ids survive `retention` rather than [`RETENTION`]. + /// + /// `Duration::ZERO` prunes strictly to the live union, which is how a test + /// asserts that dropping a watcher really does release its ids. + pub fn with_retention(retention: Duration) -> Self { + Self { + revision: AtomicU64::new(0), + buffer: Mutex::new(Buffer::default()), + notify: Notify::new(), + last: Mutex::new(HashMap::new()), + watchers: Mutex::new(HashMap::new()), + next_watcher: AtomicU64::new(0), + retention, + } + } + fn buffer(&self) -> std::sync::MutexGuard<'_, Buffer> { self.buffer.lock().unwrap_or_else(PoisonError::into_inner) } @@ -243,16 +296,26 @@ impl SessionMetaEvents { pub fn observe(&self, rows: Vec) -> usize { let mut moved = Vec::new(); { + let now = Instant::now(); let mut last = self.last.lock().unwrap_or_else(PoisonError::into_inner); for row in rows { - match last.get(&row.session_id) { - Some(before) if before == &row => {} - Some(_) => { - last.insert(row.session_id.clone(), row.clone()); - moved.push(row); + // `entry` rather than `get` + `insert`: the vacant arm writes + // through the same borrow, which a `match last.get(..)` cannot + // do on stable. + match last.entry(row.session_id.clone()) { + Entry::Occupied(mut slot) => { + let tracked = slot.get_mut(); + // The stamp is refreshed whether or not the row moved: + // it records that somebody READ this chat, which is what + // `retain_union` prunes on. + tracked.seen = now; + if tracked.row != row { + tracked.row = row.clone(); + moved.push(row); + } } - None => { - last.insert(row.session_id.clone(), row); + Entry::Vacant(slot) => { + slot.insert(Tracked { row, seen: now }); } } } @@ -264,14 +327,46 @@ impl SessionMetaEvents { count } - /// Stop tracking chats nobody is watching any more, so a long-lived daemon's - /// map does not grow with every chat ever opened. - pub fn retain_watched(&self, watched: &[String]) { - let mut last = self.last.lock().unwrap_or_else(PoisonError::into_inner); - if last.len() <= watched.len() { - return; + /// Claim `ids` as watched for as long as the returned guard lives. + /// + /// One poll, one guard. The claim is what keeps those chats in + /// [`Self::last`] while some OTHER caller's poll prunes, and retiring it is + /// the only thing that can make them eligible for eviction. + /// + /// ⚠ **Registering can only ever GROW the union, so it prunes nothing.** + /// Pruning belongs on the drop, where a claim disappears. + #[must_use = "the ids are watched only for as long as the guard is alive"] + pub fn watch(self: &Arc, ids: &[String]) -> WatchGuard { + let token = self.next_watcher.fetch_add(1, Ordering::SeqCst); + self.watchers + .lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(token, ids.to_vec()); + WatchGuard { + events: Arc::clone(self), + token, } - last.retain(|id, _| watched.iter().any(|w| w == id)); + } + + /// Forget the chats that no live watcher holds and nothing has read within + /// [`Self::retention`], so a long-lived daemon's map does not grow with + /// every chat ever opened. + /// + /// ⚠ **The union of every live watcher, never one caller's list.** The + /// per-caller form this replaced is the two-window defect recorded on + /// [`Self::watchers`]; keeping the ids of every watcher is what makes the + /// prune agree with the map it prunes. + /// + /// The watchers lock is released before the rows lock is taken, so there is + /// no ordering between the two to remember. + fn retain_union(&self) { + let union: HashSet = { + let watchers = self.watchers.lock().unwrap_or_else(PoisonError::into_inner); + watchers.values().flatten().cloned().collect() + }; + let retention = self.retention; + let mut last = self.last.lock().unwrap_or_else(PoisonError::into_inner); + last.retain(|id, tracked| union.contains(id) || tracked.seen.elapsed() < retention); } /// Park until the revision moves past `since`, or `timeout` elapses. @@ -296,6 +391,26 @@ impl SessionMetaEvents { } } +/// One client's claim on the chats it has open, held for the life of its poll. +/// +/// Dropping it retires the claim and prunes the row map to what is left — see +/// [`SessionMetaEvents::watch`]. +pub struct WatchGuard { + events: Arc, + token: u64, +} + +impl Drop for WatchGuard { + fn drop(&mut self) { + self.events + .watchers + .lock() + .unwrap_or_else(PoisonError::into_inner) + .remove(&self.token); + self.events.retain_union(); + } +} + #[cfg(test)] mod tests { use super::*; @@ -314,8 +429,11 @@ mod tests { } } - fn events() -> SessionMetaEvents { - SessionMetaEvents::default() + /// An `Arc`, because a claim is taken through one — see + /// [`SessionMetaEvents::watch`]. Every existing test reaches the inner + /// methods through `Deref` and is unchanged by it. + fn events() -> Arc { + Arc::new(SessionMetaEvents::default()) } #[test] @@ -422,22 +540,114 @@ mod tests { #[test] fn a_chat_nobody_watches_stops_being_tracked() { - let e = events(); + // `Duration::ZERO` so the release is observable without sleeping through + // the grace period; that the grace exists at all is + // `a_row_rewritten_between_two_polls_of_one_window_is_still_reported`. + let e = Arc::new(SessionMetaEvents::with_retention(Duration::ZERO)); + let window_a = e.watch(&["s1".to_string()]); + let window_b = e.watch(&["s2".to_string()]); e.observe(vec![ row("s1", "versa_azure", "gpt-5.5", "public"), row("s2", "codex", "gpt-6-astra", "public"), ]); - e.retain_watched(&["s1".to_string()]); - // s2 is a stranger again, so re-observing it adopts silently rather than - // announcing a change nobody asked about. + + drop(window_b); + // s2 was nobody's the moment B retired, so it is a stranger again and + // re-observing it adopts silently rather than announcing a change + // nobody asked about. assert_eq!(e.observe(vec![row("s2", "ollama", "qwen3.6", "public")]), 0); - // s1 is still tracked. + // s1 is still claimed by A, and B retiring must not have touched it. assert_eq!(e.observe(vec![row("s1", "ollama", "qwen3.6", "public")]), 1); + + drop(window_a); + assert_eq!( + e.observe(vec![row("s1", "codex", "gpt-6-astra", "public")]), + 0, + "the last claim on s1 retired, so s1 is a stranger too" + ); + } + + #[test] + fn two_windows_with_different_open_chats_do_not_evict_each_other() { + // Two Biorouter windows, each with one subscription and its own open + // chats. The per-caller prune this replaced made them thrash: B's prune + // dropped s1, A's next `observe` re-adopted s1 as first-seen and + // published nothing, then A's prune dropped s2 — so neither window ever + // learned that a row had moved. Measured before the fix as + // "window A must learn that s1 moved; got 0". + let e = events(); + let window_a = e.watch(&["s1".to_string()]); + let window_b = e.watch(&["s2".to_string()]); + + // Each window's first sight of its own chat is silent. + assert_eq!( + e.observe(vec![row("s1", "versa_azure", "gpt-5.5", "public")]), + 0 + ); + assert_eq!( + e.observe(vec![row("s2", "codex", "gpt-6-astra", "public")]), + 0 + ); + // Interleaved rounds: the defect needed one poll from each window to + // show, and a single round would have passed against it. + for _ in 0..3 { + assert_eq!( + e.observe(vec![row("s1", "versa_azure", "gpt-5.5", "public")]), + 0 + ); + assert_eq!( + e.observe(vec![row("s2", "codex", "gpt-6-astra", "public")]), + 0 + ); + } + + // `biorouter session --resume s1 --provider claude_code` from another + // process: only window A's chat moved. + assert_eq!( + e.observe(vec![row("s1", "claude_code", "claude-opus-5", "public")]), + 1, + "window A must learn that s1 moved" + ); + assert_eq!( + e.observe(vec![row("s2", "codex", "gpt-6-astra", "public")]), + 0, + "and window B must not be told about a chat that did not move" + ); + + let delta = e.since(0); + assert_eq!(delta.changes.len(), 1); + assert_eq!(delta.changes[0].session_id, "s1"); + drop((window_a, window_b)); + } + + #[test] + fn a_row_rewritten_between_two_polls_of_one_window_is_still_reported() { + // A claim lives for exactly one parked GET, so the ONLY window's ids are + // unclaimed for an instant at every request boundary. Pruning strictly + // to the live union there would evict them, and `observe` re-adopts a + // first-seen id SILENTLY — so a row rewritten in that instant would be + // swallowed for good. `RETENTION` is what closes it, and the default + // feed is the one production uses. + let e = events(); + let poll_one = e.watch(&["s1".to_string()]); + assert_eq!( + e.observe(vec![row("s1", "versa_azure", "gpt-5.5", "public")]), + 0 + ); + drop(poll_one); + + let poll_two = e.watch(&["s1".to_string()]); + assert_eq!( + e.observe(vec![row("s1", "claude_code", "claude-opus-5", "public")]), + 1, + "a row rewritten between two polls of one window must still be reported" + ); + drop(poll_two); } #[tokio::test] async fn a_parked_poll_wakes_on_the_change_it_was_waiting_for() { - let e = Arc::new(events()); + let e = events(); e.observe(vec![row("s1", "versa_azure", "gpt-5.5", "public")]); let waiter = Arc::clone(&e); let parked = tokio::spawn(async move { From 51eecabe2c223148cab1fb25390abb86b3ed6f4d Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Wed, 9 Sep 2026 16:52:41 -0700 Subject: [PATCH 2/3] fix(test-guard): walk crates/*/tests, and state project hooks on the config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The table-driven guard that replaced three per-key instruments walked `crates/*/src/**` only, and its own doc comment explained why `tests/` was skipped — so a consolidation that read as a strict improvement narrowed the coverage by 129 of the workspace's 762 `.rs` files. The reasoning was half right: a crate's top-level `tests/` file compiles to its own binary, so a write there cannot race the lib tests. It races every other test in that binary, and nothing else in this repository looks there. Two unrestored `set_var("BIOROUTER_ALLOW_PROJECT_HOOKS", "1")` writes sat in `tests/hooks_agent_loop_tests.rs` and `tests/global_memory_consent_agent_loop.rs` while the guard, the audit's ledger and the audit's writer recipe all reported the key as handled. Both now state the decision on `AgentConfig::with_project_hooks`, the seam #205 added for exactly this; both files write a real `.biorouter/hooks.yaml`, so each binary's reader was sensitive from its first agent onward. The walk now covers `src/**` and each crate's top-level `tests/**`, with a separate non-vacuity floor per scope — one total is satisfied by the `src` half alone, so it would pass on the very narrowing this undoes. `examples/` and `build.rs` (8 files) stay out: neither is compiled into a test binary. Measured: with one `set_var` line restored, the widened guard fails naming `biorouter/tests/hooks_agent_loop_tests.rs:137`, and the pre-widening walk stays green. --- crates/biorouter/src/model.rs | 68 ++++++++++++++----- .../tests/global_memory_consent_agent_loop.rs | 12 ++-- .../biorouter/tests/hooks_agent_loop_tests.rs | 12 ++-- docs/testing/process-global-state.md | 14 +++- 4 files changed, 78 insertions(+), 28 deletions(-) diff --git a/crates/biorouter/src/model.rs b/crates/biorouter/src/model.rs index 01a821860..f8b73dc61 100644 --- a/crates/biorouter/src/model.rs +++ b/crates/biorouter/src/model.rs @@ -1172,8 +1172,8 @@ mod tests { struct Watched { key: &'static str, verdict: Verdict, - /// How many files under `crates/*/src` must still name this key **in - /// code**. + /// How many files under `crates/*/src` or `crates/*/tests` must still + /// name this key **in code**. /// /// ⚠ A **mention** count, not a match count, and deliberately so: a /// `Presence` row is healthy at zero matches, so a floor on matches @@ -1364,10 +1364,27 @@ mod tests { /// watchable set is the whole config surface, not this table. This table /// is a list of measured hazards, not a closed set. Green here is not a /// proof of hermeticity. - /// 3. **`crates/*/tests/`, which is skipped on purpose.** Each file there - /// compiles to its own binary, so a write cannot reach the lib test - /// binary these readers live in. It can still reach the other tests in - /// its own file; that is a smaller, separate hazard the audit records. + /// 3. **`crates/*/examples/` and `build.rs`.** Neither is compiled into a + /// test binary, so a write there cannot reach one at all. Eight files, + /// and the only `.rs` in the workspace this walk does not read. + /// + /// # What it walks + /// + /// `crates/*/src/**` **and** `crates/*/tests/**`. + /// + /// ⚠ **The second half was missing, and the omission read as deliberate.** + /// The two guards this table replaced walked all of `crates/**`; the table + /// walked `crates/*/src/**` and its own documentation explained why + /// `tests/` was skipped — so consolidating three instruments into one + /// narrowed the coverage by 129 of the workspace's 762 `.rs` files while + /// looking like a strict improvement. The reasoning was half right: a + /// crate's top-level `tests/` file compiles to its OWN binary, so a write + /// there cannot race the lib tests most of these readers live in. It races + /// every other test in that binary, which is a smaller hazard and not a + /// different one — and it is invisible from anywhere else, which is the + /// part that matters. Two unrestored writes of + /// `BIOROUTER_ALLOW_PROJECT_HOOKS` sat there while this guard, the audit's + /// ledger and the audit's recipe all reported the key as handled. #[test] fn no_test_parks_a_shared_setting_in_the_process_environment() { // CARGO_MANIFEST_DIR is /crates/biorouter; go up twice. @@ -1410,7 +1427,8 @@ mod tests { }) .collect(); - let mut scanned = 0usize; + let mut scanned_src = 0usize; + let mut scanned_tests = 0usize; let mut mentions = vec![0usize; WATCHED.len()]; let mut offenders: Vec = Vec::new(); @@ -1427,18 +1445,26 @@ mod tests { if path.extension().and_then(|e| e.to_str()) != Some("rs") { continue; } - // In scope: `crates//src/**`. A `tests/` directory nested - // INSIDE `src` is an ordinary module of that crate's lib and stays - // in scope; only a crate's top-level `tests/` is its own binary. + // In scope: `crates//src/**` and `crates//tests/**`. + // A `tests/` directory nested INSIDE `src` is an ordinary module of + // that crate's lib and is already covered by the first arm. + // + // Counted per scope rather than in one total, because one number + // cannot tell "the tests half was walked" from "the src half grew" — + // and narrowing back to `src` alone is exactly the regression the + // widening exists to prevent. let Ok(relative) = path.strip_prefix(&crates) else { continue; }; let mut parts = relative.components(); let _crate_name = parts.next(); - if parts.next().map(|c| c.as_os_str()) != Some(std::ffi::OsStr::new("src")) { - continue; + match parts.next().and_then(|c| c.as_os_str().to_str()) { + Some("src") => scanned_src += 1, + Some("tests") => scanned_tests += 1, + // `examples/` and `build.rs`: not compiled into a test binary, + // so a write there cannot reach one. + _ => continue, } - scanned += 1; let Ok(source) = std::fs::read_to_string(path) else { continue; }; @@ -1496,11 +1522,19 @@ mod tests { } } - // A walk that reads nothing agrees with a walk that finds nothing. + // A walk that reads nothing agrees with a walk that finds nothing. One + // floor per scope: a single total is satisfied by the `src` half alone, + // so it would pass on the very narrowing this widening undid. + assert!( + scanned_src > 400, + "the audit only scanned {scanned_src} files under crates/*/src, which is too few \ + to have walked the workspace" + ); assert!( - scanned > 400, - "the audit only scanned {scanned} files under crates/*/src, which is too few to \ - have walked the workspace" + scanned_tests > 90, + "the audit only scanned {scanned_tests} files under crates/*/tests, which is too \ + few to have walked them. A write parked there is invisible to every other test in \ + its own binary, and to every other instrument in this repository" ); // PER-KEY non-vacuity. One global floor lets most rows rot silently diff --git a/crates/biorouter/tests/global_memory_consent_agent_loop.rs b/crates/biorouter/tests/global_memory_consent_agent_loop.rs index 82e43d55c..551bf1d4b 100644 --- a/crates/biorouter/tests/global_memory_consent_agent_loop.rs +++ b/crates/biorouter/tests/global_memory_consent_agent_loop.rs @@ -136,9 +136,12 @@ async fn agent_with_hooks( provider: Arc, mode: BioRouterMode, ) -> (Agent, String, TempDir) { - // SAFETY: only flips the project-hook opt-in the HooksManager reads when it - // is constructed, a few lines below. - std::env::set_var("BIOROUTER_ALLOW_PROJECT_HOOKS", "1"); + // The project-hooks decision is STATED on the agent's config below rather + // than parked in the process environment. `HooksManager::new_with_managed` + // reads `BIOROUTER_ALLOW_PROJECT_HOOKS` with a bare `std::env::var` that no + // lock and no task-local override can reach, and this file never removed the + // variable — so it stood for every agent built in this binary afterwards. + // See `docs/testing/process-global-state.md`. let work_dir = TempDir::new().unwrap(); std::fs::create_dir_all(work_dir.path().join(".biorouter")).unwrap(); @@ -155,7 +158,8 @@ async fn agent_with_hooks( Arc::new(PermissionManager::new(permission_dir.path().to_path_buf())), None, mode, - ); + ) + .with_project_hooks(true); let agent = Agent::with_config(config); let session = session_manager diff --git a/crates/biorouter/tests/hooks_agent_loop_tests.rs b/crates/biorouter/tests/hooks_agent_loop_tests.rs index 3ba4940d3..9ab173552 100644 --- a/crates/biorouter/tests/hooks_agent_loop_tests.rs +++ b/crates/biorouter/tests/hooks_agent_loop_tests.rs @@ -127,9 +127,12 @@ async fn agent_with_project_hooks_in_mode( provider: Arc, mode: BioRouterMode, ) -> (Agent, String, TempDir) { - // SAFETY: tests run single-threaded per process for this env var; it only - // flips on the project-hook opt-in the HooksManager reads at construction. - std::env::set_var("BIOROUTER_ALLOW_PROJECT_HOOKS", "1"); + // The project-hooks decision is STATED on the agent's config below rather + // than parked in the process environment. `HooksManager::new_with_managed` + // reads `BIOROUTER_ALLOW_PROJECT_HOOKS` with a bare `std::env::var` that no + // lock and no task-local override can reach, and this file never removed the + // variable — so it stood for every agent built in this binary afterwards. + // See `docs/testing/process-global-state.md`. let work_dir = TempDir::new().unwrap(); std::fs::create_dir_all(work_dir.path().join(".biorouter")).unwrap(); @@ -149,7 +152,8 @@ async fn agent_with_project_hooks_in_mode( Arc::new(PermissionManager::new(permission_dir.path().to_path_buf())), None, mode, - ); + ) + .with_project_hooks(true); let agent = Agent::with_config(config); let session = session_manager diff --git a/docs/testing/process-global-state.md b/docs/testing/process-global-state.md index bc746320c..1338ed11a 100644 --- a/docs/testing/process-global-state.md +++ b/docs/testing/process-global-state.md @@ -27,7 +27,7 @@ Two remedies exist and **they are not interchangeable**: - **No new locks.** `env-lock` is a single global mutex over the whole environment. Adding a holder serialises writers against writers, which was never the failure mode, and does nothing about the unlocked readers that are. - **No new `#[serial]`.** An *unkeyed* `#[serial]` is worth even less than it looks: it excludes a test from the 31 other unkeyed ones and leaves it concurrent with the rest. -- **A source-scan guard beats a stress run.** The race needs an interleaving CI produces and a loaded laptop may never show, so twenty green runs are weak evidence. A source scan cannot flake. Give every scan a non-vacuity floor and prove it against a deliberately poisoned probe before trusting it. The standing guard is `model::tests::no_test_parks_a_shared_setting_in_the_process_environment` ([#209](https://github.com/BaranziniLab/biorouter/pull/209)), one table covering nine keys on both principles. +- **A source-scan guard beats a stress run.** The race needs an interleaving CI produces and a loaded laptop may never show, so twenty green runs are weak evidence. A source scan cannot flake. Give every scan a non-vacuity floor and prove it against a deliberately poisoned probe before trusting it. The standing guard is `model::tests::no_test_parks_a_shared_setting_in_the_process_environment` ([#209](https://github.com/BaranziniLab/biorouter/pull/209)), one table covering nine keys on both principles. It walks `crates/*/src/**` **and** `crates/*/tests/**`. ⚠ The `tests/` half was added later: the two guards that table replaced had walked all of `crates/**`, so consolidating three instruments into one silently dropped 129 of the workspace's 762 `.rs` files, and a consolidation is exactly the kind of change nobody re-measures the coverage of. - **Writing the environment is not banned outright.** A key that is *test-private* — namespaced so no production reader can resolve it — and restored on drop is safe without any lock, because there is no reader to race. ## Why this document exists rather than a fifth per-key guard @@ -119,8 +119,9 @@ Verdicts: **fixed**, **live** (a reader can observe another test's write today), | `TEST_KEY`, `API_KEY`, `PROVIDER`, `PORT`, `ENABLED`, `CONFIG`, `TEST_PRECEDENCE` | none in production today; `get_param` upper-cases, so any `get_param("provider")` would resolve one | 11 unguarded bare `set_var` in 3 `config/base.rs` tests; `TEST_KEY` was never removed at all | **fixed** — [#199](https://github.com/BaranziniLab/biorouter/pull/199): namespaced to `BIOROUTER_TEST_CONFIG_*` and restored on drop | | `OSV_ENDPOINT` | `OsvChecker::new` (`agents/extension_malware_check.rs:18`), reached in production from `extension_manager.rs:972` on every Stdio extension install | 3 tests, RAII-restored but unkeyed `#[serial]` | **live** — fixed by [#205](https://github.com/BaranziniLab/biorouter/pull/205) | | `BIOROUTER_TOOL_CALL_BATCHING` | `providers/base.rs:1236`, bare `env::var`, once per streamed turn | `formats/anthropic.rs:1745` under `#[serial(tool_call_batching_env)]`, a key only 3 tests hold | **live** — 5 flag-sensitive tests hold no key; fixed by [#205](https://github.com/BaranziniLab/biorouter/pull/205) | -| `BIOROUTER_ALLOW_PROJECT_HOOKS` | `hooks/mod.rs:214`, bare `env::var` | `providers/bedrock.rs:1147`, **never removed** | **latent** — no `.biorouter/hooks.yaml` in-tree, so no reader is sensitive today; fixed by [#205](https://github.com/BaranziniLab/biorouter/pull/205) | +| `BIOROUTER_ALLOW_PROJECT_HOOKS` | `hooks/mod.rs:214`, bare `env::var` | three writers, **none of them ever removed**: `providers/bedrock.rs:1147` in the lib, and `tests/hooks_agent_loop_tests.rs:132` + `tests/global_memory_consent_agent_loop.rs:141` in two separate test binaries | **fixed** — the lib writer by [#205](https://github.com/BaranziniLab/biorouter/pull/205), the two `tests/` writers by the row below. ⚠ This row said "no `.biorouter/hooks.yaml` in-tree, so no reader is sensitive today", and that was true only of the lib: **both** test files write one into their own working directory, so each binary's reader is sensitive from its first agent onward. Neither file ever removed the variable, so the opt-in stands for every agent that binary builds afterwards — including `agent_with_memory`, which asks for `hooks: {}` and never wanted the unlock. | | `BIOROUTER_ALLOW_PROJECT_HOOKS` override | `hooks/mod.rs:214` | `agents/subagent_tool.rs:5663` via `with_config_overrides` | **live defect, not a race** — the override is a no-op, so that arm does not test its own unlock; fixed by [#205](https://github.com/BaranziniLab/biorouter/pull/205) | +| `BIOROUTER_ALLOW_PROJECT_HOOKS` in `crates/*/tests` | `hooks/mod.rs:214` | `tests/hooks_agent_loop_tests.rs:132`, `tests/global_memory_consent_agent_loop.rs:141` | **fixed** — both now state the decision on `AgentConfig::with_project_hooks`, the seam [#205](https://github.com/BaranziniLab/biorouter/pull/205) added for exactly this. ⚠ **The interesting part is not the defect, it is that three instruments reported it clean.** The standing guard walked `crates/*/src` only, the row above named one writer, and the writer recipe under [Re-measuring](#re-measuring) grepped `crates/biorouter/src` — so the key read as handled from every angle while both writes stood. Measured: restoring one of the two lines leaves the pre-widening guard green. | | `HOME` | `security/policy/command.rs:846`, `policy/target.rs:125` | `knowledge/conversation_ingest.rs:806` | **latent** — `global_memory.rs` is already mitigated by `pinned_store_root()` | | `HOME` via `dirs::home_dir()` | `config/search_path.rs:72` and `:109`, both production, inside `SearchPaths::builder()` | the same `conversation_ingest.rs:806` `env_lock` writer | **live** — `a_coding_agent_path_offers_the_node_version_managers` reads `HOME` itself at `:214` to build its expected paths, then the builder reads it again; two instants, and the test holds no lock | | `BIOROUTER_PATH_ROOT` (the general case) | ~46 live `Paths::config_dir()` readers | 33 `lock_env` writers | **open** — deferred, see below | @@ -171,8 +172,15 @@ grep -rn 'Paths::[a-z_]*(' crates/biorouter/src --include='*.rs' grep -rn 'env::var\(_os\)\?(\s*"' crates/biorouter/src --include='*.rs' ``` +⚠ **This one scans all of `crates/`, and the scope is the point.** It read +`crates/biorouter/src` until 2026-09-09, matching the standing guard's walk — which +is how two unrestored writers of `BIOROUTER_ALLOW_PROJECT_HOOKS` sat in +`crates/biorouter/tests/` unreported. A writer in a crate's top-level `tests/` +cannot reach the lib test binary, but it reaches every other test in its own +binary, and nothing else in this repository looks there. + ```bash -grep -rn 'std::env::\(set_var\|remove_var\)' crates/biorouter/src --include='*.rs' +grep -rn 'std::env::\(set_var\|remove_var\)' crates/ --include='*.rs' ``` ```bash From 41957f66f9796563910ac1630a1bf9dc3fce2520 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Wed, 9 Sep 2026 17:14:06 -0700 Subject: [PATCH 3/3] test(session-meta): pin the one line that carries the watcher claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every behavioural test in the workspace passes with the claim deleted: the row map lives in `biorouter`, the poll takes its guard in `biorouter-server`, and no test drives the route. So the line that carries the fix had no instrument at all — the same shape as the widened guard in the commit before this one, one layer down. A source scan is the only thing that can see it. Two traps this repository has already measured apply here and both are handled: the scan reads the production slice only, because this module's own tests name the shape they forbid; and it strips comments, because the call site carries a warning that SPELLS `let _ = events.watch(&ids)` — an unstripped scan finds two claims and fails on a correct tree. Proved against both poisoned probes rather than reasoned about. With `let _ = events.watch(&ids)`: "the claim must be bound to a NAMED local". With the line deleted: "the poll claims its ids exactly once … Found: []". --- .../src/routes/session_meta.rs | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/crates/biorouter-server/src/routes/session_meta.rs b/crates/biorouter-server/src/routes/session_meta.rs index 27e398327..11d6faee7 100644 --- a/crates/biorouter-server/src/routes/session_meta.rs +++ b/crates/biorouter-server/src/routes/session_meta.rs @@ -201,4 +201,53 @@ mod tests { .join(","); assert_eq!(parse_ids(Some(&many)).len(), MAX_IDS); } + + /// The claim must be BOUND, and nothing that RUNS can see whether it is. + /// + /// ⚠ This is the one line carrying the two-window fix, and every + /// behavioural test in the workspace passes without it: the row map lives + /// in `biorouter`, the poll's claim is taken here, and a route test drives + /// neither. Deleting `let _claim = …` restores the defect, and so does + /// writing `let _ = …`, which drops the guard on the spot while reading as + /// a fix. A source scan is the only instrument that can see either. + #[test] + fn the_poll_binds_its_watch_claim_for_the_life_of_the_request() { + // Production only. This module names the shape it forbids, so a scan + // that read its own tests would report itself as its first offender. + let (production, _) = include_str!("session_meta.rs") + .split_once("#[cfg(test)]") + .expect("this route's tests sit at the end, behind one `#[cfg(test)]`"); + assert!( + production.len() > 5000, + "the production slice is {} bytes, far too short to be this route — the slice is wrong and a clean result would mean nothing", + production.len() + ); + + // Comments are stripped for the same reason, and it is not theoretical + // here: the call site carries a warning that SPELLS the forbidden + // `let _ = …` form, so an unstripped scan sees two claims and fails on + // a correct tree. Truncating early can only lose a match, and a lost + // match fails the count below rather than passing quietly. + let claims: Vec<&str> = production + .lines() + .filter_map(|line| line.split("//").next()) + .filter(|code| code.contains(".watch(")) + .map(str::trim) + .collect(); + assert_eq!( + claims.len(), + 1, + "the poll claims its ids exactly once, for the life of the request. Found: {claims:?}" + ); + + // Whitespace-insensitive, so `let _=` cannot slip past a spelling. + let squashed: String = claims[0].chars().filter(|c| !c.is_whitespace()).collect(); + assert!( + squashed.starts_with("let") && !squashed.starts_with("let_="), + "the claim must be bound to a NAMED local. `let _ = …` drops the guard \ + immediately, which prunes the process-global row map against this one caller's \ + ids again — the defect this endpoint had, wearing a fix. Found: {}", + claims[0] + ); + } }